| Message ID | 20171027142855.21584-24-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marc, Hi Marc, On 27/10/2017 16:28, Marc Zyngier wrote: > The GICv4 architecture doesn't make it easy for save/restore to > work, as it doesn't give any guarantee that the pending state > is written into the pending table. I don't understand where does the limitation exactly come from. Can't we use the GICR_VPENDBASER table data? Thanks Eric > > So let's not take any chance, and let's return an error if > we encounter any LPI that has the HW bit set. In order for > userspace to distinguish this error from other failure modes, > use -EACCES as an error code. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > Documentation/virtual/kvm/devices/arm-vgic-its.txt | 2 ++ > virt/kvm/arm/vgic/vgic-its.c | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt > index eb06beb75960..1eec7bcb2d52 100644 > --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt > +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt > @@ -60,6 +60,8 @@ Groups: > -EINVAL: Inconsistent restored data > -EFAULT: Invalid guest ram access > -EBUSY: One or more VCPUS are running > + -EACCES: The virtual ITS is backed by a physical GICv4 ITS, and the > + state is not available > > KVM_DEV_ARM_VGIC_GRP_ITS_REGS > Attributes: > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index eb72eb027060..2710834d0920 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1989,6 +1989,15 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device) > list_for_each_entry(ite, &device->itt_head, ite_list) { > gpa_t gpa = base + ite->event_id * ite_esz; > > + /* > + * If an LPI carries the HW bit, this means that this > + * interrupt is controlled by GICv4, and we do not > + * have direct access to that state. Let's simply fail > + * the save operation... > + */ > + if (ite->irq->hw) > + return -EACCES; > + > ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz); > if (ret) > return ret; >
On 07/11/17 15:24, Auger Eric wrote: > Hi Marc, > > Hi Marc, > On 27/10/2017 16:28, Marc Zyngier wrote: >> The GICv4 architecture doesn't make it easy for save/restore to >> work, as it doesn't give any guarantee that the pending state >> is written into the pending table. > > I don't understand where does the limitation exactly come from. Can't we > use the GICR_VPENDBASER table data? You can't. None of the tables that are written by either the ITS or the redistributors are architected. All you know is that there is one bit per vLPI, but that's it (you don't even know which one is which). But that's not a big deal. I don't think you can realistically migrate a VM that has a directly assigned device anyway. Or can we? Thanks, M.
Hi Marc, On 07/11/2017 16:38, Marc Zyngier wrote: > On 07/11/17 15:24, Auger Eric wrote: >> Hi Marc, >> >> Hi Marc, >> On 27/10/2017 16:28, Marc Zyngier wrote: >>> The GICv4 architecture doesn't make it easy for save/restore to >>> work, as it doesn't give any guarantee that the pending state >>> is written into the pending table. >> >> I don't understand where does the limitation exactly come from. Can't we >> use the GICR_VPENDBASER table data? > You can't. None of the tables that are written by either the ITS or the > redistributors are architected. All you know is that there is one bit > per vLPI, but that's it (you don't even know which one is which). Oh I thought the redistributor config and pending tables were fully specified (6.1.2 and 6.1.3 of the spec), except the 1kB of the pending table. > > But that's not a big deal. I don't think you can realistically migrate a > VM that has a directly assigned device anyway. Or can we? No we can't except for mediated devices for which migration can be supported. Thanks Eric > > Thanks, > > M. >
On 07/11/17 16:12, Auger Eric wrote: > Hi Marc, > > On 07/11/2017 16:38, Marc Zyngier wrote: >> On 07/11/17 15:24, Auger Eric wrote: >>> Hi Marc, >>> >>> Hi Marc, >>> On 27/10/2017 16:28, Marc Zyngier wrote: >>>> The GICv4 architecture doesn't make it easy for save/restore to >>>> work, as it doesn't give any guarantee that the pending state >>>> is written into the pending table. >>> >>> I don't understand where does the limitation exactly come from. Can't we >>> use the GICR_VPENDBASER table data? >> You can't. None of the tables that are written by either the ITS or the >> redistributors are architected. All you know is that there is one bit >> per vLPI, but that's it (you don't even know which one is which). > Oh I thought the redistributor config and pending tables were fully > specified (6.1.2 and 6.1.3 of the spec), except the 1kB of the pending > table. The property table is definitely architected. It is a lot less clear for the pending table. The main issue is that you cannot really know when the various bits have actually been flushed all the way from the redistributor caches to memory to be introspected. Yes, it sucks. Thanks, M.
Hi On 07/11/2017 17:34, Marc Zyngier wrote: > On 07/11/17 16:12, Auger Eric wrote: >> Hi Marc, >> >> On 07/11/2017 16:38, Marc Zyngier wrote: >>> On 07/11/17 15:24, Auger Eric wrote: >>>> Hi Marc, >>>> >>>> Hi Marc, >>>> On 27/10/2017 16:28, Marc Zyngier wrote: >>>>> The GICv4 architecture doesn't make it easy for save/restore to >>>>> work, as it doesn't give any guarantee that the pending state >>>>> is written into the pending table. >>>> >>>> I don't understand where does the limitation exactly come from. Can't we >>>> use the GICR_VPENDBASER table data? >>> You can't. None of the tables that are written by either the ITS or the >>> redistributors are architected. All you know is that there is one bit >>> per vLPI, but that's it (you don't even know which one is which). >> Oh I thought the redistributor config and pending tables were fully >> specified (6.1.2 and 6.1.3 of the spec), except the 1kB of the pending >> table. > > The property table is definitely architected. It is a lot less clear for > the pending table. The main issue is that you cannot really know when > the various bits have actually been flushed all the way from the > redistributor caches to memory to be introspected. Yes, it sucks. Oh OK the INV only guarantees the caches are consistent with the LPI config table. Maybe you could clarify the commit message with those details. So Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > > Thanks, > > M. >
Hi, On 07/11/2017 23:24, Auger Eric wrote: > Hi > > On 07/11/2017 17:34, Marc Zyngier wrote: >> On 07/11/17 16:12, Auger Eric wrote: >>> Hi Marc, >>> >>> On 07/11/2017 16:38, Marc Zyngier wrote: >>>> On 07/11/17 15:24, Auger Eric wrote: >>>>> Hi Marc, >>>>> >>>>> Hi Marc, >>>>> On 27/10/2017 16:28, Marc Zyngier wrote: >>>>>> The GICv4 architecture doesn't make it easy for save/restore to >>>>>> work, as it doesn't give any guarantee that the pending state >>>>>> is written into the pending table. >>>>> >>>>> I don't understand where does the limitation exactly come from. Can't we >>>>> use the GICR_VPENDBASER table data? >>>> You can't. None of the tables that are written by either the ITS or the >>>> redistributors are architected. All you know is that there is one bit >>>> per vLPI, but that's it (you don't even know which one is which). >>> Oh I thought the redistributor config and pending tables were fully >>> specified (6.1.2 and 6.1.3 of the spec), except the 1kB of the pending >>> table. >> >> The property table is definitely architected. It is a lot less clear for >> the pending table. The main issue is that you cannot really know when >> the various bits have actually been flushed all the way from the >> redistributor caches to memory to be introspected. Yes, it sucks. > Oh OK the INV only guarantees the caches are consistent with the LPI > config table. Maybe you could clarify the commit message with those details. > > So > Reviewed-by: Eric Auger <eric.auger@redhat.com> Reading the spec further, looks setting the Valid bit of GICR_VPENDBASER is supposed to force the redistributor to retrieve pending interrupts from the the VCPU I/F and ensure the VPT in memory is correct. Anyway that's not a big deal at the moment as you pointed out... Thanks Eric > > Thanks > > Eric > >> >> Thanks, >> >> M. >>
diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt index eb06beb75960..1eec7bcb2d52 100644 --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt @@ -60,6 +60,8 @@ Groups: -EINVAL: Inconsistent restored data -EFAULT: Invalid guest ram access -EBUSY: One or more VCPUS are running + -EACCES: The virtual ITS is backed by a physical GICv4 ITS, and the + state is not available KVM_DEV_ARM_VGIC_GRP_ITS_REGS Attributes: diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index eb72eb027060..2710834d0920 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1989,6 +1989,15 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device) list_for_each_entry(ite, &device->itt_head, ite_list) { gpa_t gpa = base + ite->event_id * ite_esz; + /* + * If an LPI carries the HW bit, this means that this + * interrupt is controlled by GICv4, and we do not + * have direct access to that state. Let's simply fail + * the save operation... + */ + if (ite->irq->hw) + return -EACCES; + ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz); if (ret) return ret;
The GICv4 architecture doesn't make it easy for save/restore to work, as it doesn't give any guarantee that the pending state is written into the pending table. So let's not take any chance, and let's return an error if we encounter any LPI that has the HW bit set. In order for userspace to distinguish this error from other failure modes, use -EACCES as an error code. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- Documentation/virtual/kvm/devices/arm-vgic-its.txt | 2 ++ virt/kvm/arm/vgic/vgic-its.c | 9 +++++++++ 2 files changed, 11 insertions(+)