Re: [PATCH v4 4/6] xen/vpci: header: status register handler

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/30/23 10:05, Jan Beulich wrote:
> On 28.08.2023 19:56, Stewart Hildebrand wrote:
>> --- 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;
>> +}
> 
> Imo we also cannot validly pass through any of the reserved bits. Doing so
> is an option only once we know what purpose they might gain.

OK. I think in the long term, having a res_mask in struct vpci_register for the 
reserved bits will be more flexible.

> (In this
> context I notice our set of PCI_STATUS_* constants isn't quite up-to-date.)

I'll add these 2 new constants in the next version of the series (in a separate 
patch):
#define  PCI_STATUS_IMM_READY  0x01    /* Immediate Readiness */

#define  PCI_STATUS_INTERRUPT  0x08    /* Interrupt status */

>> @@ -544,6 +556,11 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>      if ( rc )
>>          return rc;
>>
>> +    rc = vpci_add_rw1c_register(pdev->vpci, status_read, vpci_hw_write16,
>> +                                PCI_STATUS, 2, header, 0xF900);
> 
> Rather than a literal number, imo this wants to be an OR of the respective
> PCI_STATUS_* constants (which, if you like, could of course be consolidated
> into a new PCI_STATUS_RW1C_MASK, to help readability).

OK.

>> @@ -167,6 +174,7 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
>> *read_handler,
>>      r->size = size;
>>      r->offset = offset;
>>      r->private = data;
>> +    r->rw1c_mask = rw1c_mask;
> 
> To avoid surprises with ...
> 
>> @@ -424,6 +443,7 @@ static void vpci_write_helper(const struct pci_dev *pdev,
>>          uint32_t val;
>>
>>          val = r->read(pdev, r->offset, r->private);
>> +        val &= ~r->rw1c_mask;
>>          data = merge_result(val, data, size, offset);
> 
> ... the user of this field, should you either assert that no bits beyond
> the field size are set, or simply mask to the respective number of bits?

Good point, I'll mask it (in add_register()).

Stew