Re: [PATCH v5 4/5] xen/vpci: header: status register handler

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2023 23:29, Stewart Hildebrand wrote:
> On 9/6/23 04:22, Jan Beulich wrote:
>> On 01.09.2023 06:57, Stewart Hildebrand wrote:
>>> Introduce a handler for the PCI status register, with ability to mask the
>>> capabilities bit. The status register contains reserved bits, read-only 
>>> bits,
>>> and write-1-to-clear bits, so introduce bitmasks to handle these in vPCI. 
>>> If a
>>> bit in the bitmask is set, then the special meaning applies:
>>>
>>>   res_mask: read as zero, write ignore
>>>   ro_mask: read normal, write ignore
>>>   rw1c_mask: read normal, write 1 to clear
>>
>> With the last one's name being descriptive of what the behavior is, would
>> it make sense to name the first one "raz_mask"? (Also a question to Roger
>> as the maintainer of this code.)
> 
> Another possible name is rsvdz_mask. See below.

Ah, yes, that's better even if for now we won't need rsvdp support.

>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -413,6 +413,18 @@ static void cf_check cmd_write(
>>>          pci_conf_write16(pdev->sbdf, reg, cmd);
>>>  }
>>>
>>> +static uint32_t cf_check status_read(const struct pci_dev *pdev,
>>> +                                     unsigned int reg, void *data)
>>> +{
>>> +    struct vpci_header *header = data;
>>> +    uint32_t status = pci_conf_read16(pdev->sbdf, reg);
>>> +
>>> +    if ( header->mask_cap_list )
>>> +        status &= ~PCI_STATUS_CAP_LIST;
>>> +
>>> +    return status;
>>> +}
>>
>> Do you actually need this function? Can't you ...
>>
>>> @@ -544,6 +556,12 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>>      if ( rc )
>>>          return rc;
>>>
>>> +    rc = vpci_add_register_mask(pdev->vpci, status_read, vpci_hw_write16,
>>> +                                PCI_STATUS, 2, header, 
>>> PCI_STATUS_RESERVED_MASK,
>>> +                                PCI_STATUS_RO_MASK, PCI_STATUS_RW1C_MASK);
>>
>> ... conditionally OR in PCI_STATUS_CAP_LIST right here? Without
>> capabilities the CAP_LIST bit becomes kind of reserved anyway.
> 
> Hm. The PCI_STATUS_CAP_LIST bit is a read-only bit according to spec. Given 
> the rationale below, we would want to preserve the value of r/o bits during 
> writes, so this would essentially want to become a RsvdP bit. I wasn't 
> planning on adding support for RsvdP bits here since there aren't any proper 
> RsvdP bits in the status register. If instead we are okay with treating the 
> PCI_STATUS_CAP_LIST bit as RsvdZ, then we could do as you suggest, but I'll 
> plan to keep it as is for now.

I'd say leverage rsvdz (with a comment) for the time being, but let's see
what Roger thinks.

>>> @@ -424,9 +450,13 @@ static void vpci_write_helper(const struct pci_dev 
>>> *pdev,
>>>          uint32_t val;
>>>
>>>          val = r->read(pdev, r->offset, r->private);
>>> +        val &= ~r->res_mask;
>>> +        val &= ~r->rw1c_mask;
>>
>> Personally I'd fold these two lines into just one (and similarly below).
> 
> Will do.
> 
>>>          data = merge_result(val, data, size, offset);
>>>      }
>>>
>>> +    data &= ~r->res_mask;
>>> +    data &= ~r->ro_mask;
>>
>> This seems risky to me. I'd rather see the same value being written back
>> for r/o bits, just in case a device doesn't actually implement a bit as
>> mandated. 
> 
> Regarding writes to read-only bits: okay, I'll assume that Xen should take 
> care of preserving the r/o bits (discarding the guests write value for the 
> r/o bits).
> 
> If, on the other hand, we would want to rely on the guest to preserve the r/o 
> bits during a write, then the ro_mask would effectively not be doing 
> anything, so may as well be removed.

We may never rely on the guest doing anything the way we want it.

> In either case, for partial register writes, Xen will take care of preserving 
> the r/o bits for the remaining bytes in the register.
> 
> I'll make this change for the next version of the series (assuming Xen will 
> handle preserving the r/o bits).
> 
>> For reserved flags it's less clear what's best, because in
>> principle they could also become rw1c once defined.
> 
> Well, it depends on what version of the spec we read.
> 
> PCI Local Bus 3.0 spec section 6.1 says: "On writes, software must ensure 
> that the values of reserved bit positions are preserved." PCI Local Bus 3.0 
> spec section 6.2.3 also says that we can essentially treat the whole status 
> register as rw1c. Huh, that's odd.
> 
> PCI Express Base 4.0 spec defines two reserved bit types:
> RsvdP: Reserved and Preserved - Reserved for future RW implementations. 
> Software must preserve the value read for writes to bits.
> RsvdZ: Reserved and Zero - Reserved for future RW1C implementations. Software 
> must use 0b for writes to bits.
> Both RsvdP and RsvdZ are read as zero.
> 
> I'm favoring using the definitions in the newer PCI Express Base 4.0 spec 
> since they are clearer.

+1

> Regarding writes to rsvdp bits: there are none of these in the status 
> register according to the PCI Express Base 4.0 spec. The older PCI Local Bus 
> 3.0 spec is in conflict with this definition, but at the same time the PCI 
> Local Bus 3.0 spec also conflicts with itself by saying that the entire 
> status register is essentially a rw1c register with reserved bits that should 
> be preserved, which doesn't make sense. The newer PCI Express Base 4.0 spec 
> is clearer, and appears to have resolved this inconsistency, so I'm favoring 
> adhering to the newer PCI Express Base 4.0 spec.
> 
> Regarding writes to rsvdz bits: in this case Xen will write zero to the rsvdz 
> bits (discarding the guests write value). This is how the patch already 
> behaves with the res_mask, but I think renaming it to rsvdz_mask will make it 
> more clear, so I'm inclined to rename it for the next revision of the series.
> 
> Regarding reads of rsvdp/rsvdz bits: I'm assuming that Xen should return zero 
> to the guest for reads of reserved bits, regardless of what is actually read 
> from the hardware. I think we already are already on the same page about 
> this, I just wanted to explicitly state it for clarity.

Yes, we are.

Jan