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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.

>> --- 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.

>> @@ -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.

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.

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.

>> --- a/xen/include/xen/pci_regs.h
>> +++ b/xen/include/xen/pci_regs.h
>> @@ -66,6 +66,14 @@
>>  #define  PCI_STATUS_REC_MASTER_ABORT 0x2000 /* Set on master abort */
>>  #define  PCI_STATUS_SIG_SYSTEM_ERROR 0x4000 /* Set when we drive SERR */
>>  #define  PCI_STATUS_DETECTED_PARITY  0x8000 /* Set on parity error */
>> +#define  PCI_STATUS_RESERVED_MASK    0x06
> 
> I'd recommend separating the "derived" constants by a blank line. I'd
> further like to ask that you add two more padding zeros above.

Okay, will do.

>> +#define  PCI_STATUS_RO_MASK (PCI_STATUS_IMM_READY | PCI_STATUS_INTERRUPT | \
>> +    PCI_STATUS_CAP_LIST | PCI_STATUS_CAP_LIST | PCI_STATUS_66MHZ | \
> 
> CAP_LIST twice?

Good catch, it obviously only needs to be there once. I will fix.