* Re: [PATCH] PCI: export and use pci_msi_get_hwirq in pci-hyperv.c
From: Rob Herring @ 2020-05-07 20:58 UTC (permalink / raw)
To: Wei Liu
Cc: linux-pci, linux-hyperv, linux-kernel, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, Allison Randal,
Greg Kroah-Hartman, Enrico Weigelt
In-Reply-To: <20200422195818.35489-1-wei.liu@kernel.org>
On Wed, Apr 22, 2020 at 07:58:15PM +0000, Wei Liu wrote:
> There is a functionally identical function in pci-hyperv.c. Drop it and
> use pci_msi_get_hwirq instead.
>
> This requires exporting pci_msi_get_hwirq and declaring it in msi.h.
>
> No functional change intended.
>
> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> ---
> arch/x86/include/asm/msi.h | 4 ++++
> arch/x86/kernel/apic/msi.c | 5 +++--
> drivers/pci/controller/pci-hyperv.c | 8 +-------
> 3 files changed, 8 insertions(+), 9 deletions(-)
Would be better if done in a way to remove an x86 dependency.
I guess this would do it:
#define pci_msi_get_hwirq NULL
when GENERIC_MSI_DOMAIN_OPS is enabled.
>
> diff --git a/arch/x86/include/asm/msi.h b/arch/x86/include/asm/msi.h
> index 25ddd0916bb2..353b80122b2e 100644
> --- a/arch/x86/include/asm/msi.h
> +++ b/arch/x86/include/asm/msi.h
> @@ -11,4 +11,8 @@ int pci_msi_prepare(struct irq_domain *domain, struct device *dev, int nvec,
>
> void pci_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc);
>
> +struct msi_domain_info;
> +irq_hw_number_t pci_msi_get_hwirq(struct msi_domain_info *info,
> + msi_alloc_info_t *arg);
> +
> #endif /* _ASM_X86_MSI_H */
> diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
> index 159bd0cb8548..56dcdd912564 100644
> --- a/arch/x86/kernel/apic/msi.c
> +++ b/arch/x86/kernel/apic/msi.c
> @@ -204,11 +204,12 @@ void native_teardown_msi_irq(unsigned int irq)
> irq_domain_free_irqs(irq, 1);
> }
>
> -static irq_hw_number_t pci_msi_get_hwirq(struct msi_domain_info *info,
> - msi_alloc_info_t *arg)
> +irq_hw_number_t pci_msi_get_hwirq(struct msi_domain_info *info,
> + msi_alloc_info_t *arg)
> {
> return arg->msi_hwirq;
> }
> +EXPORT_SYMBOL_GPL(pci_msi_get_hwirq);
>
> int pci_msi_prepare(struct irq_domain *domain, struct device *dev, int nvec,
> msi_alloc_info_t *arg)
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e6020480a28b..2b4a6452095f 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1520,14 +1520,8 @@ static struct irq_chip hv_msi_irq_chip = {
> .irq_unmask = hv_irq_unmask,
> };
>
> -static irq_hw_number_t hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
> - msi_alloc_info_t *arg)
> -{
> - return arg->msi_hwirq;
> -}
> -
> static struct msi_domain_ops hv_msi_ops = {
> - .get_hwirq = hv_msi_domain_ops_get_hwirq,
> + .get_hwirq = pci_msi_get_hwirq,
> .msi_prepare = pci_msi_prepare,
> .set_desc = pci_msi_set_desc,
> .msi_free = hv_msi_free,
> --
> 2.20.1
>
^ permalink raw reply
* [PATCH] vmbus: Replace zero-length array with flexible-array
From: Gustavo A. R. Silva @ 2020-05-07 18:53 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu
Cc: linux-hyperv, linux-kernel
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
sizeof(flexible-array-member) triggers a warning because flexible array
members have incomplete type[1]. There are some instances of code in
which the sizeof operator is being incorrectly/erroneously applied to
zero-length arrays and the result is zero. Such instances may be hiding
some bugs. So, this work (flexible-array member conversions) will also
help to get completely rid of those sorts of issues.
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
include/linux/hyperv.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 692c89ccf5df..ce2c27440e17 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -117,7 +117,7 @@ struct hv_ring_buffer {
* Ring data starts here + RingDataStartOffset
* !!! DO NOT place any fields below this !!!
*/
- u8 buffer[0];
+ u8 buffer[];
} __packed;
struct hv_ring_buffer_info {
@@ -313,7 +313,7 @@ struct vmadd_remove_transfer_page_set {
struct gpa_range {
u32 byte_count;
u32 byte_offset;
- u64 pfn_array[0];
+ u64 pfn_array[];
};
/*
@@ -563,7 +563,7 @@ struct vmbus_channel_gpadl_header {
u32 gpadl;
u16 range_buflen;
u16 rangecount;
- struct gpa_range range[0];
+ struct gpa_range range[];
} __packed;
/* This is the followup packet that contains more PFNs. */
@@ -571,7 +571,7 @@ struct vmbus_channel_gpadl_body {
struct vmbus_channel_message_header header;
u32 msgnumber;
u32 gpadl;
- u64 pfn[0];
+ u64 pfn[];
} __packed;
struct vmbus_channel_gpadl_created {
@@ -672,7 +672,7 @@ struct vmbus_channel_msginfo {
* The channel message that goes out on the "wire".
* It will contain at minimum the VMBUS_CHANNEL_MESSAGE_HEADER header
*/
- unsigned char msg[0];
+ unsigned char msg[];
};
struct vmbus_close_msg {
^ permalink raw reply related
* Re: [PATCH] uio_hv_generic: add missed sysfs_remove_bin_file
From: Greg Kroah-Hartman @ 2020-05-07 16:36 UTC (permalink / raw)
To: Chuhong Yuan
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, linux-kernel
In-Reply-To: <20200507151343.792816-1-hslester96@gmail.com>
On Thu, May 07, 2020 at 11:13:43PM +0800, Chuhong Yuan wrote:
> This driver calls sysfs_create_bin_file() in probe, but forgets to
> call sysfs_remove_bin_file() in remove.
> Add the missed call to fix it.
>
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
> drivers/uio/uio_hv_generic.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 3c5169eb23f5..4dae2320b103 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -361,6 +361,7 @@ hv_uio_remove(struct hv_device *dev)
> if (!pdata)
> return 0;
>
> + sysfs_remove_bin_file(&dev->channel->kobj, &ring_buffer_bin_attr);
> uio_unregister_device(&pdata->info);
> hv_uio_cleanup(dev, pdata);
> hv_set_drvdata(dev, NULL);
I'll take this, but it's not always needed as all sysfs files are
removed from the device when it is removed from sysfs anyway. So this
shouldn't be an issue in a real system.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] uio_hv_generic: add missed sysfs_remove_bin_file
From: Chuhong Yuan @ 2020-05-07 15:13 UTC (permalink / raw)
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Greg Kroah-Hartman, linux-hyperv, linux-kernel, Chuhong Yuan
This driver calls sysfs_create_bin_file() in probe, but forgets to
call sysfs_remove_bin_file() in remove.
Add the missed call to fix it.
Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
drivers/uio/uio_hv_generic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index 3c5169eb23f5..4dae2320b103 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -361,6 +361,7 @@ hv_uio_remove(struct hv_device *dev)
if (!pdata)
return 0;
+ sysfs_remove_bin_file(&dev->channel->kobj, &ring_buffer_bin_attr);
uio_unregister_device(&pdata->info);
hv_uio_cleanup(dev, pdata);
hv_set_drvdata(dev, NULL);
--
2.26.2
^ permalink raw reply related
* RE: [PATCH v3 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Michael Kelley @ 2020-05-07 14:29 UTC (permalink / raw)
To: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, lorenzo.pieralisi@arm.com, robh@kernel.org,
bhelgaas@google.com, linux-hyperv@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Dexuan Cui
In-Reply-To: <20200507050211.10923-1-weh@microsoft.com>
From: Wei Hu <weh@microsoft.com> Sent: Wednesday, May 6, 2020 10:02 PM
>
> In some error cases in hv_pci_probe(), allocated resources are not freed.
> Fix this by adding a field to keep track of the high water mark for slots
> that have resources allocated to them. In case of an error, this high
> water mark is used to know which slots have resources that must be released.
> Since slots are numbered starting with zero, a value of -1 indicates no
> slots have been allocated resources. There may be unused slots in the range
> between slot 0 and the high water mark slot, but these slots are already
> ignored by the existing code in the allocate and release loops with the call
> to get_pcichild_wslot().
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
> v3: Add detailed explanation of this patch in commit log per Lorenzo
> Pieralisi's suggestions.
>
> drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e15022ff63e3..e6fac0187722 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -480,6 +480,9 @@ struct hv_pcibus_device {
>
> struct workqueue_struct *wq;
>
> + /* Highest slot of child device with resources allocated */
> + int wslot_res_allocated;
> +
> /* hypercall arg, must not cross page boundary */
> struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> @@ -2847,7 +2850,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> struct hv_pci_dev *hpdev;
> struct pci_packet *pkt;
> size_t size_res;
> - u32 wslot;
> + int wslot;
> int ret;
>
> size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
> @@ -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
> comp_pkt.completion_status);
> break;
> }
> +
> + hbus->wslot_res_allocated = wslot;
> }
>
> kfree(pkt);
> @@ -2918,10 +2923,10 @@ static int hv_send_resources_released(struct hv_device *hdev)
> struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> struct pci_child_message pkt;
> struct hv_pci_dev *hpdev;
> - u32 wslot;
> + int wslot;
> int ret;
>
> - for (wslot = 0; wslot < 256; wslot++) {
> + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
> hpdev = get_pcichild_wslot(hbus, wslot);
> if (!hpdev)
> continue;
> @@ -2936,8 +2941,12 @@ static int hv_send_resources_released(struct hv_device *hdev)
> VM_PKT_DATA_INBAND, 0);
> if (ret)
> return ret;
> +
> + hbus->wslot_res_allocated = wslot - 1;
> }
>
> + hbus->wslot_res_allocated = -1;
> +
> return 0;
> }
>
> @@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> if (!hbus)
> return -ENOMEM;
> hbus->state = hv_pcibus_init;
> + hbus->wslot_res_allocated = -1;
>
> /*
> * The PCI bus "domain" is what is called "segment" in ACPI and other
> @@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> ret = hv_pci_allocate_bridge_windows(hbus);
> if (ret)
> - goto free_irq_domain;
> + goto exit_d0;
>
> ret = hv_send_resources_allocated(hdev);
> if (ret)
> @@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> free_windows:
> hv_pci_free_bridge_windows(hbus);
> +exit_d0:
> + (void) hv_pci_bus_exit(hdev, true);
> free_irq_domain:
> irq_domain_remove(hbus->irq_domain);
> free_fwnode:
> --
> 2.20.1
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* RE: [PATCH v3 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Michael Kelley @ 2020-05-07 14:30 UTC (permalink / raw)
To: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, lorenzo.pieralisi@arm.com, robh@kernel.org,
bhelgaas@google.com, linux-hyperv@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Dexuan Cui
In-Reply-To: <20200507050300.10974-1-weh@microsoft.com>
From: Wei Hu <weh@microsoft.com> Sent: Wednesday, May 6, 2020 10:03 PM
>
> In the case of kdump, the PCI device was not cleanly shut down
> before the kdump kernel starts. This causes the initial
> attempt of entering D0 state in the kdump kernel to fail with
> invalid device state returned from Hyper-V host.
> When this happens, explicitly call PCI bus exit and retry to
> enter the D0 state.
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
> v2: Incorporate review comments from Michael Kelley, Dexuan Cui and
> Bjorn Helgaas
>
> drivers/pci/controller/pci-hyperv.c | 40 +++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e6fac0187722..92092a47d3af 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2739,6 +2739,8 @@ static void hv_free_config_window(struct hv_pcibus_device
> *hbus)
> vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
> }
>
> +static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs);
> +
> /**
> * hv_pci_enter_d0() - Bring the "bus" into the D0 power state
> * @hdev: VMBus's tracking struct for this root PCI bus
> @@ -2751,8 +2753,10 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> struct pci_bus_d0_entry *d0_entry;
> struct hv_pci_compl comp_pkt;
> struct pci_packet *pkt;
> + bool retry = true;
> int ret;
>
> +enter_d0_retry:
> /*
> * Tell the host that the bus is ready to use, and moved into the
> * powered-on state. This includes telling the host which region
> @@ -2779,6 +2783,38 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> if (ret)
> goto exit;
>
> + /*
> + * In certain case (Kdump) the pci device of interest was
> + * not cleanly shut down and resource is still held on host
> + * side, the host could return invalid device status.
> + * We need to explicitly request host to release the resource
> + * and try to enter D0 again.
> + */
> + if (comp_pkt.completion_status < 0 && retry) {
> + retry = false;
> +
> + dev_err(&hdev->device, "Retrying D0 Entry\n");
> +
> + /*
> + * Hv_pci_bus_exit() calls hv_send_resource_released()
> + * to free up resources of its child devices.
> + * In the kdump kernel we need to set the
> + * wslot_res_allocated to 255 so it scans all child
> + * devices to release resources allocated in the
> + * normal kernel before panic happened.
> + */
> + hbus->wslot_res_allocated = 255;
> +
> + ret = hv_pci_bus_exit(hdev, true);
> +
> + if (ret == 0) {
> + kfree(pkt);
> + goto enter_d0_retry;
> + }
> + dev_err(&hdev->device,
> + "Retrying D0 failed with ret %d\n", ret);
> + }
> +
> if (comp_pkt.completion_status < 0) {
> dev_err(&hdev->device,
> "PCI Pass-through VSP failed D0 Entry with status %x\n",
> @@ -3185,7 +3221,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> return ret;
> }
>
> -static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
> +static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs)
> {
> struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> struct {
> @@ -3203,7 +3239,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool
> hibernating)
> if (hdev->channel->rescind)
> return 0;
>
> - if (!hibernating) {
> + if (!keep_devs) {
> /* Delete any children which might still exist. */
> dr = kzalloc(sizeof(*dr), GFP_KERNEL);
> if (dr && hv_pci_start_relations_work(hbus, dr))
> --
> 2.20.1
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [PATCH] Driver: hv: vmbus: drop a no long applicable comment
From: Wei Liu @ 2020-05-07 10:36 UTC (permalink / raw)
To: Michael Kelley
Cc: Wei Liu, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger
In-Reply-To: <MW2PR2101MB10521E94593C54B9E806CCBDD7A40@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, May 06, 2020 at 05:49:05PM +0000, Michael Kelley wrote:
> From: Wei Liu <wei.liu@kernel.org> Sent: Wednesday, May 6, 2020 9:08 AM
> >
> > None of the things mentioned in the comment is initialized in hv_init.
> > They've been moved elsewhere.
> >
> > Signed-off-by: Wei Liu <wei.liu@kernel.org>
> > ---
> > drivers/hv/vmbus_drv.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 3a27f6c5f3de..7efdcadc335e 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1396,7 +1396,6 @@ static int vmbus_bus_init(void)
> > {
> > int ret;
> >
> > - /* Hypervisor initialization...setup hypercall page..etc */
> > ret = hv_init();
> > if (ret != 0) {
> > pr_err("Unable to initialize the hypervisor - 0x%x\n", ret);
> > --
> > 2.20.1
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>
Thanks.
Applied to hyperv-next.
^ permalink raw reply
* Re: [PATCH v11 0/7] x86/kvm/hyper-v: add support for synthetic debugger
From: Paolo Bonzini @ 2020-05-07 7:57 UTC (permalink / raw)
To: Jon Doron, kvm, linux-hyperv; +Cc: vkuznets
In-Reply-To: <20200507030141.GF2862@jondnuc>
On 07/05/20 05:01, Jon Doron wrote:
> Paolo was this merged in or by any chance in the queue?
No, I'll get to it today.
Paolo
> Thanks,
> -- Jon.
>
> On 24/04/2020, Jon Doron wrote:
>> Add support for the synthetic debugger interface of hyper-v, the
>> synthetic debugger has 2 modes.
>> 1. Use a set of MSRs to send/recv information (undocumented so it's not
>> going to the hyperv-tlfs.h)
>> 2. Use hypercalls
>>
>> The first mode is based the following MSRs:
>> 1. Control/Status MSRs which either asks for a send/recv .
>> 2. Send/Recv MSRs each holds GPA where the send/recv buffers are.
>> 3. Pending MSR, holds a GPA to a PAGE that simply has a boolean that
>> indicates if there is data pending to issue a recv VMEXIT.
>>
>> The first mode implementation is to simply exit to user-space when
>> either the control MSR or the pending MSR are being set.
>> Then it's up-to userspace to implement the rest of the logic of
>> sending/recving.
>>
>> In the second mode instead of using MSRs KNet will simply issue
>> Hypercalls with the information to send/recv, in this mode the data
>> being transferred is UDP encapsulated, unlike in the previous mode in
>> which you get just the data to send.
>>
>> The new hypercalls will exit to userspace which will be incharge of
>> re-encapsulating if needed the UDP packets to be sent.
>>
>> There is an issue though in which KDNet does not respect the hypercall
>> page and simply issues vmcall/vmmcall instructions depending on the cpu
>> type expecting them to be handled as it a real hypercall was issued.
>>
>> It's important to note that part of this feature has been subject to be
>> removed in future versions of Windows, which is why some of the
>> defintions will not be present the the TLFS but in the kvm hyperv header
>> instead.
>>
>> v11:
>> Fixed all reviewed by and rebased on latest origin/master
>>
>> Jon Doron (6):
>> x86/kvm/hyper-v: Explicitly align hcall param for kvm_hyperv_exit
>> x86/kvm/hyper-v: Simplify addition for custom cpuid leafs
>> x86/hyper-v: Add synthetic debugger definitions
>> x86/kvm/hyper-v: Add support for synthetic debugger capability
>> x86/kvm/hyper-v: enable hypercalls without hypercall page with syndbg
>> x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls
>>
>> Vitaly Kuznetsov (1):
>> KVM: selftests: update hyperv_cpuid with SynDBG tests
>>
>> Documentation/virt/kvm/api.rst | 18 ++
>> arch/x86/include/asm/hyperv-tlfs.h | 6 +
>> arch/x86/include/asm/kvm_host.h | 14 +
>> arch/x86/kvm/hyperv.c | 242 ++++++++++++++++--
>> arch/x86/kvm/hyperv.h | 33 +++
>> arch/x86/kvm/trace.h | 51 ++++
>> arch/x86/kvm/x86.c | 13 +
>> include/uapi/linux/kvm.h | 13 +
>> .../selftests/kvm/x86_64/hyperv_cpuid.c | 143 +++++++----
>> 9 files changed, 468 insertions(+), 65 deletions(-)
>>
>> --
>> 2.24.1
>>
>
^ permalink raw reply
* [PATCH v3 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Wei Hu @ 2020-05-07 5:03 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, lorenzo.pieralisi, robh,
bhelgaas, linux-hyperv, linux-pci, linux-kernel, decui, mikelley
Cc: Wei Hu
In the case of kdump, the PCI device was not cleanly shut down
before the kdump kernel starts. This causes the initial
attempt of entering D0 state in the kdump kernel to fail with
invalid device state returned from Hyper-V host.
When this happens, explicitly call PCI bus exit and retry to
enter the D0 state.
Signed-off-by: Wei Hu <weh@microsoft.com>
---
v2: Incorporate review comments from Michael Kelley, Dexuan Cui and
Bjorn Helgaas
drivers/pci/controller/pci-hyperv.c | 40 +++++++++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index e6fac0187722..92092a47d3af 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -2739,6 +2739,8 @@ static void hv_free_config_window(struct hv_pcibus_device *hbus)
vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
}
+static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs);
+
/**
* hv_pci_enter_d0() - Bring the "bus" into the D0 power state
* @hdev: VMBus's tracking struct for this root PCI bus
@@ -2751,8 +2753,10 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
struct pci_bus_d0_entry *d0_entry;
struct hv_pci_compl comp_pkt;
struct pci_packet *pkt;
+ bool retry = true;
int ret;
+enter_d0_retry:
/*
* Tell the host that the bus is ready to use, and moved into the
* powered-on state. This includes telling the host which region
@@ -2779,6 +2783,38 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
if (ret)
goto exit;
+ /*
+ * In certain case (Kdump) the pci device of interest was
+ * not cleanly shut down and resource is still held on host
+ * side, the host could return invalid device status.
+ * We need to explicitly request host to release the resource
+ * and try to enter D0 again.
+ */
+ if (comp_pkt.completion_status < 0 && retry) {
+ retry = false;
+
+ dev_err(&hdev->device, "Retrying D0 Entry\n");
+
+ /*
+ * Hv_pci_bus_exit() calls hv_send_resource_released()
+ * to free up resources of its child devices.
+ * In the kdump kernel we need to set the
+ * wslot_res_allocated to 255 so it scans all child
+ * devices to release resources allocated in the
+ * normal kernel before panic happened.
+ */
+ hbus->wslot_res_allocated = 255;
+
+ ret = hv_pci_bus_exit(hdev, true);
+
+ if (ret == 0) {
+ kfree(pkt);
+ goto enter_d0_retry;
+ }
+ dev_err(&hdev->device,
+ "Retrying D0 failed with ret %d\n", ret);
+ }
+
if (comp_pkt.completion_status < 0) {
dev_err(&hdev->device,
"PCI Pass-through VSP failed D0 Entry with status %x\n",
@@ -3185,7 +3221,7 @@ static int hv_pci_probe(struct hv_device *hdev,
return ret;
}
-static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
+static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs)
{
struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
struct {
@@ -3203,7 +3239,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
if (hdev->channel->rescind)
return 0;
- if (!hibernating) {
+ if (!keep_devs) {
/* Delete any children which might still exist. */
dr = kzalloc(sizeof(*dr), GFP_KERNEL);
if (dr && hv_pci_start_relations_work(hbus, dr))
--
2.20.1
^ permalink raw reply related
* [PATCH v3 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Wei Hu @ 2020-05-07 5:02 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, lorenzo.pieralisi, robh,
bhelgaas, linux-hyperv, linux-pci, linux-kernel, decui, mikelley
Cc: Wei Hu
In some error cases in hv_pci_probe(), allocated resources are not freed.
Fix this by adding a field to keep track of the high water mark for slots
that have resources allocated to them. In case of an error, this high
water mark is used to know which slots have resources that must be released.
Since slots are numbered starting with zero, a value of -1 indicates no
slots have been allocated resources. There may be unused slots in the range
between slot 0 and the high water mark slot, but these slots are already
ignored by the existing code in the allocate and release loops with the call
to get_pcichild_wslot().
Signed-off-by: Wei Hu <weh@microsoft.com>
---
v3: Add detailed explanation of this patch in commit log per Lorenzo
Pieralisi's suggestions.
drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index e15022ff63e3..e6fac0187722 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -480,6 +480,9 @@ struct hv_pcibus_device {
struct workqueue_struct *wq;
+ /* Highest slot of child device with resources allocated */
+ int wslot_res_allocated;
+
/* hypercall arg, must not cross page boundary */
struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
@@ -2847,7 +2850,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
struct hv_pci_dev *hpdev;
struct pci_packet *pkt;
size_t size_res;
- u32 wslot;
+ int wslot;
int ret;
size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
@@ -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
comp_pkt.completion_status);
break;
}
+
+ hbus->wslot_res_allocated = wslot;
}
kfree(pkt);
@@ -2918,10 +2923,10 @@ static int hv_send_resources_released(struct hv_device *hdev)
struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
struct pci_child_message pkt;
struct hv_pci_dev *hpdev;
- u32 wslot;
+ int wslot;
int ret;
- for (wslot = 0; wslot < 256; wslot++) {
+ for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
hpdev = get_pcichild_wslot(hbus, wslot);
if (!hpdev)
continue;
@@ -2936,8 +2941,12 @@ static int hv_send_resources_released(struct hv_device *hdev)
VM_PKT_DATA_INBAND, 0);
if (ret)
return ret;
+
+ hbus->wslot_res_allocated = wslot - 1;
}
+ hbus->wslot_res_allocated = -1;
+
return 0;
}
@@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev,
if (!hbus)
return -ENOMEM;
hbus->state = hv_pcibus_init;
+ hbus->wslot_res_allocated = -1;
/*
* The PCI bus "domain" is what is called "segment" in ACPI and other
@@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct hv_device *hdev,
ret = hv_pci_allocate_bridge_windows(hbus);
if (ret)
- goto free_irq_domain;
+ goto exit_d0;
ret = hv_send_resources_allocated(hdev);
if (ret)
@@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device *hdev,
free_windows:
hv_pci_free_bridge_windows(hbus);
+exit_d0:
+ (void) hv_pci_bus_exit(hdev, true);
free_irq_domain:
irq_domain_remove(hbus->irq_domain);
free_fwnode:
--
2.20.1
^ permalink raw reply related
* [PATCH v3 0/2] Fix PCI HyperV device error handling
From: Wei Hu @ 2020-05-07 5:01 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, wei.liu, lorenzo.pieralisi, robh,
bhelgaas, linux-hyperv, linux-pci, linux-kernel, decui, mikelley
Cc: Wei Hu
This series better handles some PCI HyperV error cases in general
and for kdump case. Some of review comments from previous individual
patch reviews, including splitting into separate patches, have already
been incorporated. Thanks Lorenzo Pieralisi for the review and
suggestions, as well as Michael Kelley's contribution to the commit
log.
Thanks,
Wei
Wei Hu (2):
PCI: hv: Fix the PCI HyperV probe failure path to release resource
properly
PCI: hv: Retry PCI bus D0 entry when the first attempt failed with
invalid device state
drivers/pci/controller/pci-hyperv.c | 60 ++++++++++++++++++++++++++---
1 file changed, 54 insertions(+), 6 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH v11 0/7] x86/kvm/hyper-v: add support for synthetic debugger
From: Jon Doron @ 2020-05-07 3:01 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, pbonzini
In-Reply-To: <20200424113746.3473563-1-arilou@gmail.com>
Paolo was this merged in or by any chance in the queue?
Thanks,
-- Jon.
On 24/04/2020, Jon Doron wrote:
>Add support for the synthetic debugger interface of hyper-v, the
>synthetic debugger has 2 modes.
>1. Use a set of MSRs to send/recv information (undocumented so it's not
> going to the hyperv-tlfs.h)
>2. Use hypercalls
>
>The first mode is based the following MSRs:
>1. Control/Status MSRs which either asks for a send/recv .
>2. Send/Recv MSRs each holds GPA where the send/recv buffers are.
>3. Pending MSR, holds a GPA to a PAGE that simply has a boolean that
> indicates if there is data pending to issue a recv VMEXIT.
>
>The first mode implementation is to simply exit to user-space when
>either the control MSR or the pending MSR are being set.
>Then it's up-to userspace to implement the rest of the logic of sending/recving.
>
>In the second mode instead of using MSRs KNet will simply issue
>Hypercalls with the information to send/recv, in this mode the data
>being transferred is UDP encapsulated, unlike in the previous mode in
>which you get just the data to send.
>
>The new hypercalls will exit to userspace which will be incharge of
>re-encapsulating if needed the UDP packets to be sent.
>
>There is an issue though in which KDNet does not respect the hypercall
>page and simply issues vmcall/vmmcall instructions depending on the cpu
>type expecting them to be handled as it a real hypercall was issued.
>
>It's important to note that part of this feature has been subject to be
>removed in future versions of Windows, which is why some of the
>defintions will not be present the the TLFS but in the kvm hyperv header
>instead.
>
>v11:
>Fixed all reviewed by and rebased on latest origin/master
>
>Jon Doron (6):
> x86/kvm/hyper-v: Explicitly align hcall param for kvm_hyperv_exit
> x86/kvm/hyper-v: Simplify addition for custom cpuid leafs
> x86/hyper-v: Add synthetic debugger definitions
> x86/kvm/hyper-v: Add support for synthetic debugger capability
> x86/kvm/hyper-v: enable hypercalls without hypercall page with syndbg
> x86/kvm/hyper-v: Add support for synthetic debugger via hypercalls
>
>Vitaly Kuznetsov (1):
> KVM: selftests: update hyperv_cpuid with SynDBG tests
>
> Documentation/virt/kvm/api.rst | 18 ++
> arch/x86/include/asm/hyperv-tlfs.h | 6 +
> arch/x86/include/asm/kvm_host.h | 14 +
> arch/x86/kvm/hyperv.c | 242 ++++++++++++++++--
> arch/x86/kvm/hyperv.h | 33 +++
> arch/x86/kvm/trace.h | 51 ++++
> arch/x86/kvm/x86.c | 13 +
> include/uapi/linux/kvm.h | 13 +
> .../selftests/kvm/x86_64/hyperv_cpuid.c | 143 +++++++----
> 9 files changed, 468 insertions(+), 65 deletions(-)
>
>--
>2.24.1
>
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-05-07 3:00 UTC (permalink / raw)
To: Roman Kagan, Vitaly Kuznetsov, kvm, linux-hyperv
In-Reply-To: <20200506084615.GA32841@rvkaganb>
On 06/05/2020, Roman Kagan wrote:
>On Wed, May 06, 2020 at 07:49:29AM +0300, Jon Doron wrote:
>> Thanks Roman, I see your point, it's important for me to get the EDK2
>> working properly not sure why it's not working for me.
>
>As I wrote a good deal of that code I hope I should be able to help (and
>I'd be interested, too). How exactly does the "not working" look like?
>
Basically when I built the BIOS from the hv-scsi branch you pointed me
out to, the BIOS did not see the virtio-blk device to boot from, I
usually take the BIOS from (https://www.kraxel.org/repos/) but I will
try to build the latest EDK2 and see if it identifies the virtio-blk
device and boots from it, if that's the case perhaps i just need to
rebase your branch over the latest master of EDK2.
>Also I'm a bit confused as to why UEFI is critical for the work you're
>doing? Can't it be made to work with BIOS first?
>
The reason I want to have the UEFI option is because I need SecureBoot
to turn on VBS.
>> Do you know by any chance if the EDK2 hyperv patches were submitted and if
>> they were why they were not merged in?
>
>I do, as I'm probably the only one who could have submitted them :)
>
>No they were not submitted. Neither were the ones for SeaBIOS nor iPXE.
>The reason was that I had found no way to use alternative firmware with
>HyperV, so the only environment where that would be useful and testable
>was QEMU with VMBus. Therefore I thought it made no sense to submit
>them until VMBus landed in QEMU.
>
>Thanks,
>Roman.
Heh I see, well I'm really happy that you are here helping so we can try
and finally add VMBus to QEMU, I realize it's a big effort but I'm
willing to spend the time and do the required changes...
I'm working this only during my free time so things takes me longer than
usual (sorry for that..)
I will keep update on results once I get to test with latest EDK2 :)
Thanks,
-- Jon.
^ permalink raw reply
* RE: [PATCH] Driver: hv: vmbus: drop a no long applicable comment
From: Michael Kelley @ 2020-05-06 17:49 UTC (permalink / raw)
To: Wei Liu, linux-hyperv@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger
In-Reply-To: <20200506160806.118965-1-wei.liu@kernel.org>
From: Wei Liu <wei.liu@kernel.org> Sent: Wednesday, May 6, 2020 9:08 AM
>
> None of the things mentioned in the comment is initialized in hv_init.
> They've been moved elsewhere.
>
> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> ---
> drivers/hv/vmbus_drv.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 3a27f6c5f3de..7efdcadc335e 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1396,7 +1396,6 @@ static int vmbus_bus_init(void)
> {
> int ret;
>
> - /* Hypervisor initialization...setup hypercall page..etc */
> ret = hv_init();
> if (ret != 0) {
> pr_err("Unable to initialize the hypervisor - 0x%x\n", ret);
> --
> 2.20.1
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* [PATCH] Driver: hv: vmbus: drop a no long applicable comment
From: Wei Liu @ 2020-05-06 16:08 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, mikelley, Wei Liu, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger
None of the things mentioned in the comment is initialized in hv_init.
They've been moved elsewhere.
Signed-off-by: Wei Liu <wei.liu@kernel.org>
---
drivers/hv/vmbus_drv.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 3a27f6c5f3de..7efdcadc335e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1396,7 +1396,6 @@ static int vmbus_bus_init(void)
{
int ret;
- /* Hypervisor initialization...setup hypercall page..etc */
ret = hv_init();
if (ret != 0) {
pr_err("Unable to initialize the hypervisor - 0x%x\n", ret);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Lorenzo Pieralisi @ 2020-05-06 15:21 UTC (permalink / raw)
To: Michael Kelley
Cc: Wei Hu, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui
In-Reply-To: <MW2PR2101MB1052F033623F91A0FF991DE4D7A40@MW2PR2101MB1052.namprd21.prod.outlook.com>
On Wed, May 06, 2020 at 02:55:17PM +0000, Michael Kelley wrote:
[...]
> > Hv_pci_bus_exit() calls hv_send_resources_released() to release all child resources.
> > These child resources were allocated in hv_send_resources_allocated().
> > Hv_send_resources_allocated() could fail in the middle, leaving some child resources
> > allocated and rest not. Without adding this variable to record the highest slot number that
> > resource has been successfully allocated, calling hv_send_resources_released() could
> > cause spurious resource release requests being sent to hypervisor.
> >
> > This had been fine since hv_pci_bus_exit() was never called in error path before this patch
> > was
> > introduced. To add this call to clean the pci state in the error path, we need to know the
> > starting
> > point in child device that resource has not been allocated. Hence this variable
> > is used in hv_send_resources_allocated() to record this point and in
> > hv_send_resource_released() to start deallocating child resources.
> >
> > I can add to the commit log if you are fine with this explanation.
> >
>
> FWIW, I think of this patch as follows:
>
> In some error cases in hv_pci_probe(), allocated resources are not
> freed. Fix this by adding a field to keep track of the high water mark
> for slots that have resources allocated to them. In case of an error, this
> high water mark is used to know which slots have resources that
> must be released. Since slots are numbered starting with zero, a
> value of -1 indicates no slots have been allocated resources. There
> may be unused slots in the range between slot 0 and the high
> water mark slot, but these slots are already ignored by the existing code
> in the allocate and release loops with the call to get_pcichild_wslot().
That's much clearer now - please add these bits of info to the commit
log, it is essential that developers can find an explanation for a
change like this one there IMO.
Overall the code changes are fine, I am not a big fan of the (void)
cast (I don't think error codes should be ignored) but it is acceptable,
in this context.
Thank you for taking some time to review the code together.
Lorenzo
^ permalink raw reply
* RE: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Michael Kelley @ 2020-05-06 14:55 UTC (permalink / raw)
To: Wei Hu, Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui
In-Reply-To: <SG2P153MB0213216D3C150AA4758FCBB8BBA40@SG2P153MB0213.APCP153.PROD.OUTLOOK.COM>
From: Wei Hu <weh@microsoft.com> Sent: Wednesday, May 6, 2020 6:22 AM
> > -----Original Message-----
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Sent: Wednesday, May 6, 2020 7:10 PM
> > To: Wei Hu <weh@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > wei.liu@kernel.org; robh@kernel.org; bhelgaas@google.com; linux-
> > hyperv@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Dexuan Cui <decui@microsoft.com>; Michael Kelley
> > <mikelley@microsoft.com>
> > Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to
> > release resource properly
> >
> > On Wed, May 06, 2020 at 05:36:46AM +0000, Wei Hu wrote:
> > > Hi Lorenzo,
> > >
> > > Thanks for your review. Please see my comments inline.
> > >
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Sent: Tuesday, May 5, 2020 11:03 PM
> > > > To: Wei Hu <weh@microsoft.com>
> > > > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > > <haiyangz@microsoft.com>; Stephen Hemminger
> > > > <sthemmin@microsoft.com>; wei.liu@kernel.org; robh@kernel.org;
> > > > bhelgaas@google.com; linux- hyperv@vger.kernel.org;
> > > > linux-pci@vger.kernel.org; linux- kernel@vger.kernel.org; Dexuan Cui
> > > > <decui@microsoft.com>; Michael Kelley <mikelley@microsoft.com>
> > > > Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe
> > > > failure path to release resource properly
> > > >
> > > > On Fri, May 01, 2020 at 01:36:17PM +0800, Wei Hu wrote:
> > > > > Some error cases in hv_pci_probe() were not handled. Fix these
> > > > > error paths to release the resourses and clean up the state properly.
> > > >
> > > > This patch does more than that. It adds a variable to store the
> > > > number of slots actually allocated - I presume to free only allocated on slots
> > on the exit path.
> > > >
> > > > Two patches required I am afraid.
> > >
> > > Well, adding this variable is needed to make the call of "(void)
> > hv_pci_bus_exit(hdev, true)"
> >
> > I don't understand why - it is not clear from the commit log and the code,
> > please explain it since it is not obvious.
> >
> Hv_pci_bus_exit() calls hv_send_resources_released() to release all child resources.
> These child resources were allocated in hv_send_resources_allocated().
> Hv_send_resources_allocated() could fail in the middle, leaving some child resources
> allocated and rest not. Without adding this variable to record the highest slot number that
> resource has been successfully allocated, calling hv_send_resources_released() could
> cause spurious resource release requests being sent to hypervisor.
>
> This had been fine since hv_pci_bus_exit() was never called in error path before this patch
> was
> introduced. To add this call to clean the pci state in the error path, we need to know the
> starting
> point in child device that resource has not been allocated. Hence this variable
> is used in hv_send_resources_allocated() to record this point and in
> hv_send_resource_released() to start deallocating child resources.
>
> I can add to the commit log if you are fine with this explanation.
>
FWIW, I think of this patch as follows:
In some error cases in hv_pci_probe(), allocated resources are not
freed. Fix this by adding a field to keep track of the high water mark
for slots that have resources allocated to them. In case of an error, this
high water mark is used to know which slots have resources that
must be released. Since slots are numbered starting with zero, a
value of -1 indicates no slots have been allocated resources. There
may be unused slots in the range between slot 0 and the high
water mark slot, but these slots are already ignored by the existing code
in the allocate and release loops with the call to get_pcichild_wslot().
Michael
^ permalink raw reply
* RE: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Wei Hu @ 2020-05-06 13:21 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui, Michael Kelley
In-Reply-To: <20200506110930.GA31068@e121166-lin.cambridge.arm.com>
Thanks for your email. See my comments inline.
> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Wednesday, May 6, 2020 7:10 PM
> To: Wei Hu <weh@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> wei.liu@kernel.org; robh@kernel.org; bhelgaas@google.com; linux-
> hyperv@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; Dexuan Cui <decui@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to
> release resource properly
>
> On Wed, May 06, 2020 at 05:36:46AM +0000, Wei Hu wrote:
> > Hi Lorenzo,
> >
> > Thanks for your review. Please see my comments inline.
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Sent: Tuesday, May 5, 2020 11:03 PM
> > > To: Wei Hu <weh@microsoft.com>
> > > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > <haiyangz@microsoft.com>; Stephen Hemminger
> > > <sthemmin@microsoft.com>; wei.liu@kernel.org; robh@kernel.org;
> > > bhelgaas@google.com; linux- hyperv@vger.kernel.org;
> > > linux-pci@vger.kernel.org; linux- kernel@vger.kernel.org; Dexuan Cui
> > > <decui@microsoft.com>; Michael Kelley <mikelley@microsoft.com>
> > > Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe
> > > failure path to release resource properly
> > >
> > > On Fri, May 01, 2020 at 01:36:17PM +0800, Wei Hu wrote:
> > > > Some error cases in hv_pci_probe() were not handled. Fix these
> > > > error paths to release the resourses and clean up the state properly.
> > >
> > > This patch does more than that. It adds a variable to store the
> > > number of slots actually allocated - I presume to free only allocated on slots
> on the exit path.
> > >
> > > Two patches required I am afraid.
> >
> > Well, adding this variable is needed to make the call of "(void)
> hv_pci_bus_exit(hdev, true)"
>
> I don't understand why - it is not clear from the commit log and the code,
> please explain it since it is not obvious.
>
Hv_pci_bus_exit() calls hv_send_resources_released() to release all child resources.
These child resources were allocated in hv_send_resources_allocated().
Hv_send_resources_allocated() could fail in the middle, leaving some child resources
allocated and rest not. Without adding this variable to record the highest slot number that
resource has been successfully allocated, calling hv_send_resources_released() could
cause spurious resource release requests being sent to hypervisor.
This had been fine since hv_pci_bus_exit() was never called in error path before this patch was
introduced. To add this call to clean the pci state in the error path, we need to know the starting
point in child device that resource has not been allocated. Hence this variable
is used in hv_send_resources_allocated() to record this point and in
hv_send_resource_released() to start deallocating child resources.
I can add to the commit log if you are fine with this explanation.
> > at the end to work and clean up the PCI state in the failure path.
> > Just adding this variable would not make any changes. So I think it may be
> better to put them in single patch?
> >
> > >
> > > > Signed-off-by: Wei Hu <weh@microsoft.com>
> > > > ---
> > > > drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
> > > > 1 file changed, 16 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pci-hyperv.c
> > > > b/drivers/pci/controller/pci-hyperv.c
> > > > index e15022ff63e3..e6fac0187722 100644
> > > > --- a/drivers/pci/controller/pci-hyperv.c
> > > > +++ b/drivers/pci/controller/pci-hyperv.c
> > > > @@ -480,6 +480,9 @@ struct hv_pcibus_device {
> > > >
> > > > struct workqueue_struct *wq;
> > > >
> > > > + /* Highest slot of child device with resources allocated */
> > > > + int wslot_res_allocated;
> > >
> > > I don't understand why you need an int rather than a u32.
> > >
> > > Furthermore, I think a bitmap is more appropriate for what this
> > > variable is used for.
> >
> > So it can use a negative value (-1 in this case) to indicated none of
> > any resources has been allocated. Currently value between 0-255
> > indicating some resources have been allocated. Of course I can use
> > 0xffffffff to indicate that if it were u32. But it wouldn't make much difference,
> would it?
>
> It is fine by me - I would not have written it this way but it does not matter.
>
> > It would take 32 bytes for total 256 child slots in bitmap, while it
> > only takes 4 bytes using int. It is not in critical path so scanning
> > from the location one by one is not a big deal.
>
> I suggested a bitmap for correctness - a slot number may include slots that as
> far as I can read the code failed get_pcichild_slot().
>
> It is not clear what this patch is doing in this loop, that's certain.
>
Get_pcichild_wslot() just tells if a pci child device exists under this slot number. If
there is no child device, it just continue to look on the next slot number. If it does
exists, the code sends request to hypervisor for resource allocation. If it succeeds,
we update the wslot_res_allocated accordingly.
I hope this along with the explanation earlier makes it clear.
> > >
> > > > +
> > > > /* hypercall arg, must not cross page boundary */
> > > > struct hv_retarget_device_interrupt
> > > > retarget_msi_interrupt_params;
> > > >
> > > > @@ -2847,7 +2850,7 @@ static int
> > > > hv_send_resources_allocated(struct
> > > hv_device *hdev)
> > > > struct hv_pci_dev *hpdev;
> > > > struct pci_packet *pkt;
> > > > size_t size_res;
> > > > - u32 wslot;
> > > > + int wslot;
> > > > int ret;
> > > >
> > > > size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
> > > @@
> > > > -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct
> > > > hv_device
> > > *hdev)
> > > > comp_pkt.completion_status);
> > > > break;
> > > > }
> > > > +
> > > > + hbus->wslot_res_allocated = wslot;
> > > > }
> > > >
> > > > kfree(pkt);
> > > > @@ -2918,10 +2923,10 @@ static int
> > > > hv_send_resources_released(struct
> > > hv_device *hdev)
> > > > struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> > > > struct pci_child_message pkt;
> > > > struct hv_pci_dev *hpdev;
> > > > - u32 wslot;
> > > > + int wslot;
> > > > int ret;
> > > >
> > > > - for (wslot = 0; wslot < 256; wslot++) {
> > > > + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
> > > > hpdev = get_pcichild_wslot(hbus, wslot);
> > > > if (!hpdev)
> > > > continue;
> > > > @@ -2936,8 +2941,12 @@ static int
> > > > hv_send_resources_released(struct
> > > hv_device *hdev)
> > > > VM_PKT_DATA_INBAND, 0);
> > > > if (ret)
> > > > return ret;
> > > > +
> > > > + hbus->wslot_res_allocated = wslot - 1;
> > >
> > > Do you really need to set it at every loop iteration ?
> > >
> > > Moreover, I think a bitmap is better suited for what you are doing,
> > > given that you may skip some loop indexes on !hpdev.
> > >
> > The value is set in the loop whenever a resource was successfully
> > released. It could happen that it failed in the next iteration so the
> > last one which had succeeded would be recorded in this variable. It is
> > needed at the end of loop when this iteration succeeds.
>
> Ok understood.
>
> > Once again since it is not in the critical path, just using an signed
> > integer is straightforward, less error prone and a bit easier to
> > maintain than bitmap in my opinion. 😊
> >
>
> Less error prone, not sure, see above - it is your code so you choose but please
> explain this change better, it is not obvious.
I hope the explanations earlier make it a little better to understand.
>
> > > > }
> > > >
> > > > + hbus->wslot_res_allocated = -1;
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > > > if (!hbus)
> > > > return -ENOMEM;
> > > > hbus->state = hv_pcibus_init;
> > > > + hbus->wslot_res_allocated = -1;
> > > >
> > > > /*
> > > > * The PCI bus "domain" is what is called "segment" in ACPI and
> > > > other @@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct
> > > > hv_device *hdev,
> > > >
> > > > ret = hv_pci_allocate_bridge_windows(hbus);
> > > > if (ret)
> > > > - goto free_irq_domain;
> > > > + goto exit_d0;
> > > >
> > > > ret = hv_send_resources_allocated(hdev);
> > > > if (ret)
> > > > @@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device
> > > > *hdev,
> > > >
> > > > free_windows:
> > > > hv_pci_free_bridge_windows(hbus);
> > > > +exit_d0:
> > > > + (void) hv_pci_bus_exit(hdev, true);
> > >
> > > Remove the (void) cast.
> > >
> >
> > Some tools (maybe lint?) could generate error/warning messages
> > assuming the code fails to check the return value without such cast.
> > Leaving the cast in place just indicates that the return value is deliberately
> ignored.
>
> I understand that - the question is why it is OK to ignore it in this specific case.
>
Since it is already in the error path, checking the return value is not necessary.
The earlier failure point is more important to be returned to the caller.
> Maybe adding a wrapper around hv_pci_bus_exit() can help, I am fine with it,
> just trying to help.
Do you mean making the (void) cast in wrapper function? Or checking the return value
in the wrapper function and ignore it? Either way I think it might not be necessary.
Thanks,
Wei
^ permalink raw reply
* Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Lorenzo Pieralisi @ 2020-05-06 11:09 UTC (permalink / raw)
To: Wei Hu
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui, Michael Kelley
In-Reply-To: <SG2P153MB02136EA9764D340F3D81DF2DBBA40@SG2P153MB0213.APCP153.PROD.OUTLOOK.COM>
On Wed, May 06, 2020 at 05:36:46AM +0000, Wei Hu wrote:
> Hi Lorenzo,
>
> Thanks for your review. Please see my comments inline.
>
> > -----Original Message-----
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Sent: Tuesday, May 5, 2020 11:03 PM
> > To: Wei Hu <weh@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > wei.liu@kernel.org; robh@kernel.org; bhelgaas@google.com; linux-
> > hyperv@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Dexuan Cui <decui@microsoft.com>; Michael Kelley
> > <mikelley@microsoft.com>
> > Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to
> > release resource properly
> >
> > On Fri, May 01, 2020 at 01:36:17PM +0800, Wei Hu wrote:
> > > Some error cases in hv_pci_probe() were not handled. Fix these error
> > > paths to release the resourses and clean up the state properly.
> >
> > This patch does more than that. It adds a variable to store the number of slots
> > actually allocated - I presume to free only allocated on slots on the exit path.
> >
> > Two patches required I am afraid.
>
> Well, adding this variable is needed to make the call of "(void) hv_pci_bus_exit(hdev, true)"
I don't understand why - it is not clear from the commit log and the
code, please explain it since it is not obvious.
> at the end to work and clean up the PCI state in the failure path. Just adding this variable
> would not make any changes. So I think it may be better to put them in single patch?
>
> >
> > > Signed-off-by: Wei Hu <weh@microsoft.com>
> > > ---
> > > drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
> > > 1 file changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pci-hyperv.c
> > > b/drivers/pci/controller/pci-hyperv.c
> > > index e15022ff63e3..e6fac0187722 100644
> > > --- a/drivers/pci/controller/pci-hyperv.c
> > > +++ b/drivers/pci/controller/pci-hyperv.c
> > > @@ -480,6 +480,9 @@ struct hv_pcibus_device {
> > >
> > > struct workqueue_struct *wq;
> > >
> > > + /* Highest slot of child device with resources allocated */
> > > + int wslot_res_allocated;
> >
> > I don't understand why you need an int rather than a u32.
> >
> > Furthermore, I think a bitmap is more appropriate for what this variable is used
> > for.
>
> So it can use a negative value (-1 in this case) to indicated none of any resources
> has been allocated. Currently value between 0-255 indicating some resources
> have been allocated. Of course I can use 0xffffffff to indicate that if it were u32. But
> it wouldn't make much difference, would it?
It is fine by me - I would not have written it this way but it does
not matter.
> It would take 32 bytes for total 256 child slots in bitmap, while it only takes 4 bytes
> using int. It is not in critical path so scanning from the location one by one is not a big
> deal.
I suggested a bitmap for correctness - a slot number may include slots
that as far as I can read the code failed get_pcichild_slot().
It is not clear what this patch is doing in this loop, that's certain.
> >
> > > +
> > > /* hypercall arg, must not cross page boundary */
> > > struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
> > >
> > > @@ -2847,7 +2850,7 @@ static int hv_send_resources_allocated(struct
> > hv_device *hdev)
> > > struct hv_pci_dev *hpdev;
> > > struct pci_packet *pkt;
> > > size_t size_res;
> > > - u32 wslot;
> > > + int wslot;
> > > int ret;
> > >
> > > size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
> > @@
> > > -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device
> > *hdev)
> > > comp_pkt.completion_status);
> > > break;
> > > }
> > > +
> > > + hbus->wslot_res_allocated = wslot;
> > > }
> > >
> > > kfree(pkt);
> > > @@ -2918,10 +2923,10 @@ static int hv_send_resources_released(struct
> > hv_device *hdev)
> > > struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> > > struct pci_child_message pkt;
> > > struct hv_pci_dev *hpdev;
> > > - u32 wslot;
> > > + int wslot;
> > > int ret;
> > >
> > > - for (wslot = 0; wslot < 256; wslot++) {
> > > + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
> > > hpdev = get_pcichild_wslot(hbus, wslot);
> > > if (!hpdev)
> > > continue;
> > > @@ -2936,8 +2941,12 @@ static int hv_send_resources_released(struct
> > hv_device *hdev)
> > > VM_PKT_DATA_INBAND, 0);
> > > if (ret)
> > > return ret;
> > > +
> > > + hbus->wslot_res_allocated = wslot - 1;
> >
> > Do you really need to set it at every loop iteration ?
> >
> > Moreover, I think a bitmap is better suited for what you are doing, given that
> > you may skip some loop indexes on !hpdev.
> >
> The value is set in the loop whenever a resource was successfully released. It could
> happen that it failed in the next iteration so the last one which had succeeded would be
> recorded in this variable. It is needed at the end of loop when this
> iteration succeeds.
Ok understood.
> Once again since it is not in the critical path, just using an signed integer is straightforward,
> less error prone and a bit easier to maintain than bitmap in my opinion. 😊
>
Less error prone, not sure, see above - it is your code so you choose
but please explain this change better, it is not obvious.
> > > }
> > >
> > > + hbus->wslot_res_allocated = -1;
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > > if (!hbus)
> > > return -ENOMEM;
> > > hbus->state = hv_pcibus_init;
> > > + hbus->wslot_res_allocated = -1;
> > >
> > > /*
> > > * The PCI bus "domain" is what is called "segment" in ACPI and
> > > other @@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct hv_device
> > > *hdev,
> > >
> > > ret = hv_pci_allocate_bridge_windows(hbus);
> > > if (ret)
> > > - goto free_irq_domain;
> > > + goto exit_d0;
> > >
> > > ret = hv_send_resources_allocated(hdev);
> > > if (ret)
> > > @@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device *hdev,
> > >
> > > free_windows:
> > > hv_pci_free_bridge_windows(hbus);
> > > +exit_d0:
> > > + (void) hv_pci_bus_exit(hdev, true);
> >
> > Remove the (void) cast.
> >
>
> Some tools (maybe lint?) could generate error/warning messages assuming the code fails
> to check the return value without such cast. Leaving the cast in place just indicates that
> the return value is deliberately ignored.
I understand that - the question is why it is OK to ignore it in this
specific case.
Maybe adding a wrapper around hv_pci_bus_exit() can help, I am fine with
it, just trying to help.
Lorenzo
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Roman Kagan @ 2020-05-06 8:46 UTC (permalink / raw)
To: Jon Doron; +Cc: Vitaly Kuznetsov, kvm, linux-hyperv
In-Reply-To: <20200506044929.GD2862@jondnuc>
On Wed, May 06, 2020 at 07:49:29AM +0300, Jon Doron wrote:
> Thanks Roman, I see your point, it's important for me to get the EDK2
> working properly not sure why it's not working for me.
As I wrote a good deal of that code I hope I should be able to help (and
I'd be interested, too). How exactly does the "not working" look like?
Also I'm a bit confused as to why UEFI is critical for the work you're
doing? Can't it be made to work with BIOS first?
> Do you know by any chance if the EDK2 hyperv patches were submitted and if
> they were why they were not merged in?
I do, as I'm probably the only one who could have submitted them :)
No they were not submitted. Neither were the ones for SeaBIOS nor iPXE.
The reason was that I had found no way to use alternative firmware with
HyperV, so the only environment where that would be useful and testable
was QEMU with VMBus. Therefore I thought it made no sense to submit
them until VMBus landed in QEMU.
Thanks,
Roman.
^ permalink raw reply
* RE: [PATCH v2 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Wei Hu @ 2020-05-06 5:47 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui, Michael Kelley
In-Reply-To: <20200505150941.GB16228@e121166-lin.cambridge.arm.com>
> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Tuesday, May 5, 2020 11:10 PM
> To: Wei Hu <weh@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> wei.liu@kernel.org; robh@kernel.org; bhelgaas@google.com; linux-
> hyperv@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; Dexuan Cui <decui@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [PATCH v2 2/2] PCI: hv: Retry PCI bus D0 entry when the first
> attempt failed with invalid device state
>
> On Fri, May 01, 2020 at 01:37:28PM +0800, Wei Hu wrote:
> > In the case of kdump, the PCI device was not cleanly shut down before
> > the kdump kernel starts. This causes the initial attempt of entering
> > D0 state in the kdump kernel to fail with invalid device state
> > returned from Hyper-V host.
> > When this happens, explicitly call PCI bus exit and retry to enter the
> > D0 state.
> >
> > Signed-off-by: Wei Hu <weh@microsoft.com>
> > ---
> > v2: Incorporate review comments from Michael Kelley, Dexuan Cui and
> > Bjorn Helgaas
> >
> > drivers/pci/controller/pci-hyperv.c | 40
> > +++++++++++++++++++++++++++--
> > 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c
> > b/drivers/pci/controller/pci-hyperv.c
> > index e6fac0187722..92092a47d3af 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -2739,6 +2739,8 @@ static void hv_free_config_window(struct
> hv_pcibus_device *hbus)
> > vmbus_free_mmio(hbus->mem_config->start,
> PCI_CONFIG_MMIO_LENGTH); }
> >
> > +static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs);
> > +
> > /**
> > * hv_pci_enter_d0() - Bring the "bus" into the D0 power state
> > * @hdev: VMBus's tracking struct for this root PCI bus
> > @@ -2751,8 +2753,10 @@ static int hv_pci_enter_d0(struct hv_device
> *hdev)
> > struct pci_bus_d0_entry *d0_entry;
> > struct hv_pci_compl comp_pkt;
> > struct pci_packet *pkt;
> > + bool retry = true;
> > int ret;
> >
> > +enter_d0_retry:
> > /*
> > * Tell the host that the bus is ready to use, and moved into the
> > * powered-on state. This includes telling the host which region @@
> > -2779,6 +2783,38 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> > if (ret)
> > goto exit;
> >
> > + /*
> > + * In certain case (Kdump) the pci device of interest was
> > + * not cleanly shut down and resource is still held on host
> > + * side, the host could return invalid device status.
> > + * We need to explicitly request host to release the resource
> > + * and try to enter D0 again.
> > + */
> > + if (comp_pkt.completion_status < 0 && retry) {
> > + retry = false;
> > +
> > + dev_err(&hdev->device, "Retrying D0 Entry\n");
> > +
> > + /*
> > + * Hv_pci_bus_exit() calls hv_send_resource_released()
> > + * to free up resources of its child devices.
> > + * In the kdump kernel we need to set the
> > + * wslot_res_allocated to 255 so it scans all child
> > + * devices to release resources allocated in the
> > + * normal kernel before panic happened.
> > + */
> > + hbus->wslot_res_allocated = 255;
>
> I'd rather write a specific function eg hv_pci_bus_shutdown() to make it explicit.
> Actually, isn't it something that should *always* be _enforced_ at host bridge
> probe time - regardless of the kernel you are booting on ?
>
It is only needed in kdump kernel when the normal kernel which just crashed
failed to deallocate the PCI resources cleanly. All the states have been taken
care of in the normal kernel without needing this.
Wei
^ permalink raw reply
* RE: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to release resource properly
From: Wei Hu @ 2020-05-06 5:36 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
wei.liu@kernel.org, robh@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Dexuan Cui, Michael Kelley
In-Reply-To: <20200505150315.GA16228@e121166-lin.cambridge.arm.com>
Hi Lorenzo,
Thanks for your review. Please see my comments inline.
> -----Original Message-----
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Tuesday, May 5, 2020 11:03 PM
> To: Wei Hu <weh@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> wei.liu@kernel.org; robh@kernel.org; bhelgaas@google.com; linux-
> hyperv@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; Dexuan Cui <decui@microsoft.com>; Michael Kelley
> <mikelley@microsoft.com>
> Subject: Re: [PATCH v2 1/2] PCI: hv: Fix the PCI HyperV probe failure path to
> release resource properly
>
> On Fri, May 01, 2020 at 01:36:17PM +0800, Wei Hu wrote:
> > Some error cases in hv_pci_probe() were not handled. Fix these error
> > paths to release the resourses and clean up the state properly.
>
> This patch does more than that. It adds a variable to store the number of slots
> actually allocated - I presume to free only allocated on slots on the exit path.
>
> Two patches required I am afraid.
Well, adding this variable is needed to make the call of "(void) hv_pci_bus_exit(hdev, true)"
at the end to work and clean up the PCI state in the failure path. Just adding this variable
would not make any changes. So I think it may be better to put them in single patch?
>
> > Signed-off-by: Wei Hu <weh@microsoft.com>
> > ---
> > drivers/pci/controller/pci-hyperv.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c
> > b/drivers/pci/controller/pci-hyperv.c
> > index e15022ff63e3..e6fac0187722 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -480,6 +480,9 @@ struct hv_pcibus_device {
> >
> > struct workqueue_struct *wq;
> >
> > + /* Highest slot of child device with resources allocated */
> > + int wslot_res_allocated;
>
> I don't understand why you need an int rather than a u32.
>
> Furthermore, I think a bitmap is more appropriate for what this variable is used
> for.
So it can use a negative value (-1 in this case) to indicated none of any resources
has been allocated. Currently value between 0-255 indicating some resources
have been allocated. Of course I can use 0xffffffff to indicate that if it were u32. But
it wouldn't make much difference, would it?
It would take 32 bytes for total 256 child slots in bitmap, while it only takes 4 bytes
using int. It is not in critical path so scanning from the location one by one is not a big
deal.
>
> > +
> > /* hypercall arg, must not cross page boundary */
> > struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
> >
> > @@ -2847,7 +2850,7 @@ static int hv_send_resources_allocated(struct
> hv_device *hdev)
> > struct hv_pci_dev *hpdev;
> > struct pci_packet *pkt;
> > size_t size_res;
> > - u32 wslot;
> > + int wslot;
> > int ret;
> >
> > size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
> @@
> > -2900,6 +2903,8 @@ static int hv_send_resources_allocated(struct hv_device
> *hdev)
> > comp_pkt.completion_status);
> > break;
> > }
> > +
> > + hbus->wslot_res_allocated = wslot;
> > }
> >
> > kfree(pkt);
> > @@ -2918,10 +2923,10 @@ static int hv_send_resources_released(struct
> hv_device *hdev)
> > struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> > struct pci_child_message pkt;
> > struct hv_pci_dev *hpdev;
> > - u32 wslot;
> > + int wslot;
> > int ret;
> >
> > - for (wslot = 0; wslot < 256; wslot++) {
> > + for (wslot = hbus->wslot_res_allocated; wslot >= 0; wslot--) {
> > hpdev = get_pcichild_wslot(hbus, wslot);
> > if (!hpdev)
> > continue;
> > @@ -2936,8 +2941,12 @@ static int hv_send_resources_released(struct
> hv_device *hdev)
> > VM_PKT_DATA_INBAND, 0);
> > if (ret)
> > return ret;
> > +
> > + hbus->wslot_res_allocated = wslot - 1;
>
> Do you really need to set it at every loop iteration ?
>
> Moreover, I think a bitmap is better suited for what you are doing, given that
> you may skip some loop indexes on !hpdev.
>
The value is set in the loop whenever a resource was successfully released. It could
happen that it failed in the next iteration so the last one which had succeeded would be
recorded in this variable. It is needed at the end of loop when this
iteration succeeds.
Once again since it is not in the critical path, just using an signed integer is straightforward,
less error prone and a bit easier to maintain than bitmap in my opinion. 😊
> > }
> >
> > + hbus->wslot_res_allocated = -1;
> > +
> > return 0;
> > }
> >
> > @@ -3037,6 +3046,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > if (!hbus)
> > return -ENOMEM;
> > hbus->state = hv_pcibus_init;
> > + hbus->wslot_res_allocated = -1;
> >
> > /*
> > * The PCI bus "domain" is what is called "segment" in ACPI and
> > other @@ -3136,7 +3146,7 @@ static int hv_pci_probe(struct hv_device
> > *hdev,
> >
> > ret = hv_pci_allocate_bridge_windows(hbus);
> > if (ret)
> > - goto free_irq_domain;
> > + goto exit_d0;
> >
> > ret = hv_send_resources_allocated(hdev);
> > if (ret)
> > @@ -3154,6 +3164,8 @@ static int hv_pci_probe(struct hv_device *hdev,
> >
> > free_windows:
> > hv_pci_free_bridge_windows(hbus);
> > +exit_d0:
> > + (void) hv_pci_bus_exit(hdev, true);
>
> Remove the (void) cast.
>
Some tools (maybe lint?) could generate error/warning messages assuming the code fails
to check the return value without such cast. Leaving the cast in place just indicates that
the return value is deliberately ignored.
Thanks,
Wei
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-05-06 4:49 UTC (permalink / raw)
To: Roman Kagan, Vitaly Kuznetsov, kvm, linux-hyperv
In-Reply-To: <20200505200010.GB400685@rvkaganb>
On 05/05/2020, Roman Kagan wrote:
>On Tue, May 05, 2020 at 01:38:21PM +0300, Jon Doron wrote:
>> On 05/05/2020, Roman Kagan wrote:
>> > On Mon, May 04, 2020 at 05:55:10PM +0200, Vitaly Kuznetsov wrote:
>> > > and it seems the default state of HV_X64_MSR_SCONTROL is '1', we should
>> > > probably do the same.
>> >
>> > This is the state the OS sees, after the firmware. You'd see the same
>> > with QEMU/KVM if you used Hyper-V-aware SeaBIOS or OVMF.
>> >
>> > > Is there any reason to *not* do this in KVM when
>> > > KVM_CAP_HYPERV_SYNIC[,2] is enabled?
>> >
>> > Yes there is: quoting Hyper-V TLFS v6.0 11.8.1:
>> >
>> > At virtual processor creation time and upon processor reset, the value
>> > of this SCONTROL (SynIC control register) is 0x0000000000000000. Thus,
>> > message queuing and event flag notifications will be disabled.
>> >
>> > And, even if we decide to violate the spec it's better done in
>> > userspace, loading the initial value and adjusting the synic state at
>> > vcpu reset.
>> >
>> > However leaving it up to the guest (firmware or OS) looks more natural
>> > to me.
>>
>> I under where you are coming from in the idea of leaving it to the OS
>
>I'm coming from the HyperV spec, see the quote above.
>
>> but I think in this specific case it does not make much sense, after
>> all HyperV has it's own proprietary BIOS which Windows assumes has
>> setup some of the MSRs, since we dont have that BIOS we need to
>> "emulate" it's behaviour.
>
>We don't have that BIOS, but we have another BIOS which does the same
>and is not proprietary. Using it allows to do synic message posting
>even with a non-compliant guest OS which doesn't properly enable
>SCONTROL on its own. (Note that there used to be no problem with this
>so far, this must be specific to your use case.)
>
>I'm failing to see why this is a stumbling block for the work you're
>doing.
>
>And I'm not convinced we need to work around a non-compliant guest with
>kludges to KVM or QEMU (including back-compat stuff as that would change
>the existing behavior), when the desired effect can be achieved with the
>existing code.
>
>Thanks,
>Roman.
Thanks Roman, I see your point, it's important for me to get the EDK2
working properly not sure why it's not working for me.
Do you know by any chance if the EDK2 hyperv patches were submitted and
if they were why they were not merged in?
Thanks,
-- Jon.
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Roman Kagan @ 2020-05-05 20:00 UTC (permalink / raw)
To: Jon Doron; +Cc: Vitaly Kuznetsov, kvm, linux-hyperv
In-Reply-To: <20200505103821.GB2862@jondnuc>
On Tue, May 05, 2020 at 01:38:21PM +0300, Jon Doron wrote:
> On 05/05/2020, Roman Kagan wrote:
> > On Mon, May 04, 2020 at 05:55:10PM +0200, Vitaly Kuznetsov wrote:
> > > and it seems the default state of HV_X64_MSR_SCONTROL is '1', we should
> > > probably do the same.
> >
> > This is the state the OS sees, after the firmware. You'd see the same
> > with QEMU/KVM if you used Hyper-V-aware SeaBIOS or OVMF.
> >
> > > Is there any reason to *not* do this in KVM when
> > > KVM_CAP_HYPERV_SYNIC[,2] is enabled?
> >
> > Yes there is: quoting Hyper-V TLFS v6.0 11.8.1:
> >
> > At virtual processor creation time and upon processor reset, the value
> > of this SCONTROL (SynIC control register) is 0x0000000000000000. Thus,
> > message queuing and event flag notifications will be disabled.
> >
> > And, even if we decide to violate the spec it's better done in
> > userspace, loading the initial value and adjusting the synic state at
> > vcpu reset.
> >
> > However leaving it up to the guest (firmware or OS) looks more natural
> > to me.
>
> I under where you are coming from in the idea of leaving it to the OS
I'm coming from the HyperV spec, see the quote above.
> but I think in this specific case it does not make much sense, after
> all HyperV has it's own proprietary BIOS which Windows assumes has
> setup some of the MSRs, since we dont have that BIOS we need to
> "emulate" it's behaviour.
We don't have that BIOS, but we have another BIOS which does the same
and is not proprietary. Using it allows to do synic message posting
even with a non-compliant guest OS which doesn't properly enable
SCONTROL on its own. (Note that there used to be no problem with this
so far, this must be specific to your use case.)
I'm failing to see why this is a stumbling block for the work you're
doing.
And I'm not convinced we need to work around a non-compliant guest with
kludges to KVM or QEMU (including back-compat stuff as that would change
the existing behavior), when the desired effect can be achieved with the
existing code.
Thanks,
Roman.
^ permalink raw reply
* Re: [PATCH v2 2/2] PCI: hv: Retry PCI bus D0 entry when the first attempt failed with invalid device state
From: Lorenzo Pieralisi @ 2020-05-05 15:09 UTC (permalink / raw)
To: Wei Hu
Cc: kys, haiyangz, sthemmin, wei.liu, robh, bhelgaas, linux-hyperv,
linux-pci, linux-kernel, decui, mikelley
In-Reply-To: <20200501053728.24740-1-weh@microsoft.com>
On Fri, May 01, 2020 at 01:37:28PM +0800, Wei Hu wrote:
> In the case of kdump, the PCI device was not cleanly shut down
> before the kdump kernel starts. This causes the initial
> attempt of entering D0 state in the kdump kernel to fail with
> invalid device state returned from Hyper-V host.
> When this happens, explicitly call PCI bus exit and retry to
> enter the D0 state.
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
> v2: Incorporate review comments from Michael Kelley, Dexuan Cui and
> Bjorn Helgaas
>
> drivers/pci/controller/pci-hyperv.c | 40 +++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index e6fac0187722..92092a47d3af 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -2739,6 +2739,8 @@ static void hv_free_config_window(struct hv_pcibus_device *hbus)
> vmbus_free_mmio(hbus->mem_config->start, PCI_CONFIG_MMIO_LENGTH);
> }
>
> +static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs);
> +
> /**
> * hv_pci_enter_d0() - Bring the "bus" into the D0 power state
> * @hdev: VMBus's tracking struct for this root PCI bus
> @@ -2751,8 +2753,10 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> struct pci_bus_d0_entry *d0_entry;
> struct hv_pci_compl comp_pkt;
> struct pci_packet *pkt;
> + bool retry = true;
> int ret;
>
> +enter_d0_retry:
> /*
> * Tell the host that the bus is ready to use, and moved into the
> * powered-on state. This includes telling the host which region
> @@ -2779,6 +2783,38 @@ static int hv_pci_enter_d0(struct hv_device *hdev)
> if (ret)
> goto exit;
>
> + /*
> + * In certain case (Kdump) the pci device of interest was
> + * not cleanly shut down and resource is still held on host
> + * side, the host could return invalid device status.
> + * We need to explicitly request host to release the resource
> + * and try to enter D0 again.
> + */
> + if (comp_pkt.completion_status < 0 && retry) {
> + retry = false;
> +
> + dev_err(&hdev->device, "Retrying D0 Entry\n");
> +
> + /*
> + * Hv_pci_bus_exit() calls hv_send_resource_released()
> + * to free up resources of its child devices.
> + * In the kdump kernel we need to set the
> + * wslot_res_allocated to 255 so it scans all child
> + * devices to release resources allocated in the
> + * normal kernel before panic happened.
> + */
> + hbus->wslot_res_allocated = 255;
I'd rather write a specific function eg hv_pci_bus_shutdown() to
make it explicit. Actually, isn't it something that should *always*
be _enforced_ at host bridge probe time - regardless of the kernel
you are booting on ?
Lorenzo
> +
> + ret = hv_pci_bus_exit(hdev, true);
> +
> + if (ret == 0) {
> + kfree(pkt);
> + goto enter_d0_retry;
> + }
> + dev_err(&hdev->device,
> + "Retrying D0 failed with ret %d\n", ret);
> + }
> +
> if (comp_pkt.completion_status < 0) {
> dev_err(&hdev->device,
> "PCI Pass-through VSP failed D0 Entry with status %x\n",
> @@ -3185,7 +3221,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> return ret;
> }
>
> -static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
> +static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs)
> {
> struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> struct {
> @@ -3203,7 +3239,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
> if (hdev->channel->rescind)
> return 0;
>
> - if (!hibernating) {
> + if (!keep_devs) {
> /* Delete any children which might still exist. */
> dr = kzalloc(sizeof(*dr), GFP_KERNEL);
> if (dr && hv_pci_start_relations_work(hbus, dr))
> --
> 2.20.1
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox