| Message ID | 20200326152438.6218-25-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add reassignable BARs and PCIE 1.1 support | expand |
On 26/03/2020 15:24, Alexandru Elisei wrote: > From PCI Local Bus Specification Revision 3.0. section 3.8 "64-Bit Bus > Extension": > > "The bandwidth requirements for I/O and configuration transactions cannot > justify the added complexity, and, therefore, only memory transactions > support 64-bit data transfers". > > Further down, the spec also describes the possible responses of a target > which has been requested to do a 64-bit transaction. Limit the transaction > to the lower 32 bits, to match the second accepted behaviour. That looks like a reasonable behaviour. AFAICS there is one code path from powerpc/spapr_pci.c which isn't covered by those limitations (rtas_ibm_write_pci_config() -> pci__config_wr() -> cfg_ops.write() -> vfio_pci_cfg_write()). Same for read. I am not sure we really care, maybe you can fix it if you like. Either way this patch seems right, so: > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Cheers, Andre > --- > pci.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/pci.c b/pci.c > index 7399c76c0819..611e2c0bf1da 100644 > --- a/pci.c > +++ b/pci.c > @@ -119,6 +119,9 @@ static bool pci_config_data_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 > { > union pci_config_address pci_config_address; > > + if (size > 4) > + size = 4; > + > pci_config_address.w = ioport__read32(&pci_config_address_bits); > /* > * If someone accesses PCI configuration space offsets that are not > @@ -135,6 +138,9 @@ static bool pci_config_data_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 > { > union pci_config_address pci_config_address; > > + if (size > 4) > + size = 4; > + > pci_config_address.w = ioport__read32(&pci_config_address_bits); > /* > * If someone accesses PCI configuration space offsets that are not > @@ -248,6 +254,9 @@ static void pci_config_mmio_access(struct kvm_cpu *vcpu, u64 addr, u8 *data, > cfg_addr.w = (u32)addr; > cfg_addr.enable_bit = 1; > > + if (len > 4) > + len = 4; > + > if (is_write) > pci__config_wr(kvm, cfg_addr, data, len); > else >
Hi, On 4/2/20 9:34 AM, André Przywara wrote: > On 26/03/2020 15:24, Alexandru Elisei wrote: >> From PCI Local Bus Specification Revision 3.0. section 3.8 "64-Bit Bus >> Extension": >> >> "The bandwidth requirements for I/O and configuration transactions cannot >> justify the added complexity, and, therefore, only memory transactions >> support 64-bit data transfers". >> >> Further down, the spec also describes the possible responses of a target >> which has been requested to do a 64-bit transaction. Limit the transaction >> to the lower 32 bits, to match the second accepted behaviour. > That looks like a reasonable behaviour. > AFAICS there is one code path from powerpc/spapr_pci.c which isn't > covered by those limitations (rtas_ibm_write_pci_config() -> > pci__config_wr() -> cfg_ops.write() -> vfio_pci_cfg_write()). > Same for read. The code compares the access size to 1, 2 and 4, so I think powerpc doesn't expect 64 bit accesses either. The change looks straightforward, I'll do it for consistency. > > I am not sure we really care, maybe you can fix it if you like. > > Either way this patch seems right, so: > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> Thanks! Alex > > Cheers, > Andre > >> --- >> pci.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/pci.c b/pci.c >> index 7399c76c0819..611e2c0bf1da 100644 >> --- a/pci.c >> +++ b/pci.c >> @@ -119,6 +119,9 @@ static bool pci_config_data_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 >> { >> union pci_config_address pci_config_address; >> >> + if (size > 4) >> + size = 4; >> + >> pci_config_address.w = ioport__read32(&pci_config_address_bits); >> /* >> * If someone accesses PCI configuration space offsets that are not >> @@ -135,6 +138,9 @@ static bool pci_config_data_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 >> { >> union pci_config_address pci_config_address; >> >> + if (size > 4) >> + size = 4; >> + >> pci_config_address.w = ioport__read32(&pci_config_address_bits); >> /* >> * If someone accesses PCI configuration space offsets that are not >> @@ -248,6 +254,9 @@ static void pci_config_mmio_access(struct kvm_cpu *vcpu, u64 addr, u8 *data, >> cfg_addr.w = (u32)addr; >> cfg_addr.enable_bit = 1; >> >> + if (len > 4) >> + len = 4; >> + >> if (is_write) >> pci__config_wr(kvm, cfg_addr, data, len); >> else >>
Hi, On 5/4/20 2:00 PM, Alexandru Elisei wrote: > Hi, > > On 4/2/20 9:34 AM, André Przywara wrote: >> On 26/03/2020 15:24, Alexandru Elisei wrote: >>> From PCI Local Bus Specification Revision 3.0. section 3.8 "64-Bit Bus >>> Extension": >>> >>> "The bandwidth requirements for I/O and configuration transactions cannot >>> justify the added complexity, and, therefore, only memory transactions >>> support 64-bit data transfers". >>> >>> Further down, the spec also describes the possible responses of a target >>> which has been requested to do a 64-bit transaction. Limit the transaction >>> to the lower 32 bits, to match the second accepted behaviour. >> That looks like a reasonable behaviour. >> AFAICS there is one code path from powerpc/spapr_pci.c which isn't >> covered by those limitations (rtas_ibm_write_pci_config() -> >> pci__config_wr() -> cfg_ops.write() -> vfio_pci_cfg_write()). >> Same for read. > The code compares the access size to 1, 2 and 4, so I think powerpc doesn't expect > 64 bit accesses either. The change looks straightforward, I'll do it for consistency. > Read the code more carefully and powerpc already limits the access size to 4 bytes: static void rtas_ibm_read_pci_config(struct kvm_cpu *vcpu, uint32_t token, uint32_t nargs, target_ulong args, uint32_t nret, target_ulong rets) { [..] if (buid != phb.buid || !dev || (size > 4)) { phb_dprintf("- cfgRd buid 0x%lx cfg addr 0x%x size %d not found\n", buid, addr.w, size); rtas_st(vcpu->kvm, rets, 0, -1); return; } pci__config_rd(vcpu->kvm, addr, &val, size); [..] } It's the same for all the functions where pci__config_{rd,wr} are called directly. So no changes are needed. Thanks, Alex
diff --git a/pci.c b/pci.c index 7399c76c0819..611e2c0bf1da 100644 --- a/pci.c +++ b/pci.c @@ -119,6 +119,9 @@ static bool pci_config_data_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 { union pci_config_address pci_config_address; + if (size > 4) + size = 4; + pci_config_address.w = ioport__read32(&pci_config_address_bits); /* * If someone accesses PCI configuration space offsets that are not @@ -135,6 +138,9 @@ static bool pci_config_data_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 { union pci_config_address pci_config_address; + if (size > 4) + size = 4; + pci_config_address.w = ioport__read32(&pci_config_address_bits); /* * If someone accesses PCI configuration space offsets that are not @@ -248,6 +254,9 @@ static void pci_config_mmio_access(struct kvm_cpu *vcpu, u64 addr, u8 *data, cfg_addr.w = (u32)addr; cfg_addr.enable_bit = 1; + if (len > 4) + len = 4; + if (is_write) pci__config_wr(kvm, cfg_addr, data, len); else
From PCI Local Bus Specification Revision 3.0. section 3.8 "64-Bit Bus Extension": "The bandwidth requirements for I/O and configuration transactions cannot justify the added complexity, and, therefore, only memory transactions support 64-bit data transfers". Further down, the spec also describes the possible responses of a target which has been requested to do a 64-bit transaction. Limit the transaction to the lower 32 bits, to match the second accepted behaviour. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- pci.c | 9 +++++++++ 1 file changed, 9 insertions(+)