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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 8/22/23 09:54, Jan Beulich wrote:
> On 22.08.2023 03:29, Stewart Hildebrand wrote:
>> Introduce a handler for the PCI status register, with ability to mask the
>> capabilities bit. The status register is write-1-to-clear, so introduce 
>> handling
>> for this type of register in vPCI.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> v2->v3:
>> * new patch
> 
> This being a prereq to the cap list filtering, I think the order of the
> patches wants to be inverted.

Will do

>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -413,6 +413,17 @@ 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;
>> +
>> +    if ( header->mask_cap_list )
>> +        return pci_conf_read16(pdev->sbdf, reg) & ~(PCI_STATUS_CAP_LIST);
> 
> No need for parentheses around a constant.

OK

>> +    return pci_conf_read16(pdev->sbdf, reg);
> 
> I think this function would better issue the read just in a single place,
> and then do any fiddling that may be needed.

OK

>> @@ -556,6 +567,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);
> 
> Is it really correct to treat the entire register as rw1c, and with write-
> through to hardware for all bits? There are reserved bit there, and -
> however likely that may seem - it's guesswork whether they would also end
> up r/o or rw1c once getting assigned some meaning.

A bit mask would make more sense, so I will change it to a bit mask. As another 
data point, qemu also uses a bit mask [1].

[1] 
https://gitlab.com/qemu-project/qemu/-/blob/v8.1.0/hw/xen/xen_pt_config_init.c?ref_type=tags#L645

>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -29,6 +29,7 @@ struct vpci_register {
>>      unsigned int offset;
>>      void *private;
>>      struct list_head node;
>> +    bool rw1c : 1;
>>  };
> 
> I'm not the maintainer of this code, so what I say here may be void, but
> generally we have blanks to the left and/or right of colons in bitfield
> declarations only when we mean to pad for alignment with other nearby
> bitfields.

OK. With the change to bit mask, this will become uint32_t rw1c_mask;

>> @@ -205,6 +213,22 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
>> *read_handler,
>>      return 0;
>>  }
>>
>> +int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
>> +                      vpci_write_t *write_handler, unsigned int offset,
>> +                      unsigned int size, void *data)
>> +{
>> +    return _vpci_add_register(vpci, read_handler, write_handler, offset, 
>> size,
>> +                              data, false);
>> +}
>> +
>> +int vpci_add_rw1c_register(struct vpci *vpci, vpci_read_t *read_handler,
>> +                           vpci_write_t *write_handler, unsigned int offset,
>> +                           unsigned int size, void *data)
>> +{
>> +    return _vpci_add_register(vpci, read_handler, write_handler, offset, 
>> size,
>> +                              data, true);
>> +}
> 
> I'm always a little irritated by local functions still retaining the
> subsystem prefix. Just add_register() for the now static helper would
> imo be enough here and overall shorter to read/type.

I will change to add_register()

>> @@ -433,9 +452,11 @@ static void vpci_write_helper(const struct pci_dev 
>> *pdev,
>>
>>      if ( size != r->size )
>>      {
>> -        uint32_t val;
>> +        uint32_t val = 0;
>> +
>> +        if ( !r->rw1c )
>> +            val = r->read(pdev, r->offset, r->private);
> 
> Along with the earlier question: Doesn't rw1c need to be a bit mask,
> not a single boolean covering the entire register?

Yes

>> @@ -99,6 +106,8 @@ struct vpci {
>>           * upon to know whether BARs are mapped into the guest p2m.
>>           */
>>          bool bars_mapped      : 1;
>> +        /* Store whether to hide all capabilities from the guest. */
>> +        bool mask_cap_list : 1;
> 
> I think the intention here is for the colons to align.

OK

Stew