| Message ID | 20230828175858.30780-5-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vPCI capabilities filtering | expand |
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. (In this context I notice our set of PCI_STATUS_* constants isn't quite up-to-date.) > @@ -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). > @@ -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? Jan
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
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 767c1ba718d7..dc8c6a66770b 100644 --- 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; +} + static void cf_check bar_write( const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data) { @@ -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); + if ( rc ) + return rc; + if ( pdev->ignore_bars ) return 0; diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 3bec9a4153da..8b26870a8a2b 100644 --- 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; + uint32_t rw1c_mask; }; #ifdef __XEN__ @@ -145,9 +146,15 @@ uint32_t cf_check vpci_hw_read32( return pci_conf_read32(pdev->sbdf, reg); } -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) +void cf_check vpci_hw_write16( + const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data) +{ + pci_conf_write16(pdev->sbdf, reg, val); +} + +static int add_register(struct vpci *vpci, vpci_read_t *read_handler, + vpci_write_t *write_handler, unsigned int offset, + unsigned int size, void *data, uint32_t rw1c_mask) { struct list_head *prev; struct vpci_register *r; @@ -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; spin_lock(&vpci->lock); @@ -193,6 +201,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 add_register(vpci, read_handler, write_handler, offset, size, data, + 0); +} + +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, uint32_t rw1c_mask) +{ + return add_register(vpci, read_handler, write_handler, offset, size, data, + rw1c_mask); +} + int vpci_remove_register(struct vpci *vpci, unsigned int offset, unsigned int size) { @@ -407,11 +431,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) /* * Perform a maybe partial write to a register. - * - * Note that this will only work for simple registers, if Xen needs to - * trap accesses to rw1c registers (like the status PCI header register) - * the logic in vpci_write will have to be expanded in order to correctly - * deal with them. */ static void vpci_write_helper(const struct pci_dev *pdev, const struct vpci_register *r, unsigned int size, @@ -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); } diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index 0b8a2a3c745b..51b1b06c2c71 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -37,6 +37,11 @@ int __must_check vpci_add_register(struct vpci *vpci, vpci_write_t *write_handler, unsigned int offset, unsigned int size, void *data); +int __must_check 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, uint32_t rw1c_mask); int __must_check vpci_remove_register(struct vpci *vpci, unsigned int offset, unsigned int size); @@ -50,6 +55,8 @@ uint32_t cf_check vpci_hw_read16( const struct pci_dev *pdev, unsigned int reg, void *data); uint32_t cf_check vpci_hw_read32( const struct pci_dev *pdev, unsigned int reg, void *data); +void cf_check vpci_hw_write16( + const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data); /* * Check for pending vPCI operations on this vcpu. Returns true if the vcpu @@ -94,6 +101,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; /* FIXME: currently there's no support for SR-IOV. */ } header;
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. The mask_cap_list flag will be set in a follow-on patch. Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- v3->v4: * move mask_cap_list setting to the capabilities patch * single pci_conf_read16 in status_read * align mask_cap_list bitfield in struct vpci_header * change to rw1c bit mask instead of treating whole register as rw1c * drop subsystem prefix on renamed add_register function v2->v3: * new patch --- xen/drivers/vpci/header.c | 17 +++++++++++++++++ xen/drivers/vpci/vpci.c | 36 ++++++++++++++++++++++++++++-------- xen/include/xen/vpci.h | 9 +++++++++ 3 files changed, 54 insertions(+), 8 deletions(-)