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
