* [PATCH 1/2] pci: hyperv: x86: Move hypercall related definitions into tlfs header
From: Boqun Feng @ 2020-01-21 1:57 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: Michael Kelley, Boqun Feng, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS
For future support of virtual PCI on non-x86 platform.
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
---
arch/x86/include/asm/hyperv-tlfs.h | 4 ++++
drivers/pci/controller/pci-hyperv.c | 6 ------
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 5f10f7f2098d..b9ebc20b2385 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -376,6 +376,7 @@ struct hv_tsc_emulation_status {
#define HVCALL_SEND_IPI_EX 0x0015
#define HVCALL_POST_MESSAGE 0x005c
#define HVCALL_SIGNAL_EVENT 0x005d
+#define HVCALL_RETARGET_INTERRUPT 0x007e
#define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
#define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
@@ -405,6 +406,9 @@ enum HV_GENERIC_SET_FORMAT {
HV_GENERIC_SET_ALL,
};
+/* Declare standard hypercall field values. */
+#define HV_PARTITION_ID_SELF ((u64)-1)
+
#define HV_HYPERCALL_RESULT_MASK GENMASK_ULL(15, 0)
#define HV_HYPERCALL_FAST_BIT BIT(16)
#define HV_HYPERCALL_VARHEAD_OFFSET 17
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 9977abff92fc..aacfcc90d929 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -406,12 +406,6 @@ struct pci_eject_response {
static int pci_ring_size = (4 * PAGE_SIZE);
-/*
- * Definitions or interrupt steering hypercall.
- */
-#define HV_PARTITION_ID_SELF ((u64)-1)
-#define HVCALL_RETARGET_INTERRUPT 0x7e
-
struct hv_interrupt_entry {
u32 source; /* 1 for MSI(-X) */
u32 reserved1;
--
2.24.1
^ permalink raw reply related
* [PATCH 2/2] pci: hyperv: Move retarget related struct definitions into tlfs
From: Boqun Feng @ 2020-01-21 1:57 UTC (permalink / raw)
To: linux-hyperv, linux-kernel
Cc: Michael Kelley, Boqun Feng, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS
In-Reply-To: <20200121015713.69691-1-boqun.feng@gmail.com>
For future support of virtual PCI on non-x86 architecture.
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
---
arch/x86/include/asm/hyperv-tlfs.h | 38 +++++++++++++++++++++++++++++
arch/x86/include/asm/mshyperv.h | 8 ++++++
drivers/pci/controller/pci-hyperv.c | 38 +++--------------------------
3 files changed, 50 insertions(+), 34 deletions(-)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index b9ebc20b2385..debe017ae748 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -912,4 +912,42 @@ struct hv_tlb_flush_ex {
struct hv_partition_assist_pg {
u32 tlb_lock_count;
};
+
+union hv_msi_entry {
+ u64 as_uint64;
+ struct {
+ u32 address;
+ u32 data;
+ } __packed;
+};
+
+struct hv_interrupt_entry {
+ u32 source; /* 1 for MSI(-X) */
+ u32 reserved1;
+ union hv_msi_entry msi_entry;
+} __packed;
+
+/*
+ * flags for hv_device_interrupt_target.flags
+ */
+#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
+#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
+
+struct hv_device_interrupt_target {
+ u32 vector;
+ u32 flags;
+ union {
+ u64 vp_mask;
+ struct hv_vpset vp_set;
+ };
+} __packed;
+
+/* HvRetargetDeviceInterrupt hypercall */
+struct hv_retarget_device_interrupt {
+ u64 partition_id;
+ u64 device_id;
+ struct hv_interrupt_entry int_entry;
+ u64 reserved2;
+ struct hv_device_interrupt_target int_target;
+} __packed __aligned(8);
#endif
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 6b79515abb82..d13319d82f6b 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -240,6 +240,14 @@ bool hv_vcpu_is_preempted(int vcpu);
static inline void hv_apic_init(void) {}
#endif
+#if IS_ENABLED(CONFIG_PCI_HYPERV)
+#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \
+do { \
+ (msi_entry)->address = (msi_desc)->msg.address_lo; \
+} while (0)
+
+#endif /* CONFIG_PCI_HYPERV */
+
#else /* CONFIG_HYPERV */
static inline void hyperv_init(void) {}
static inline void hyperv_setup_mmu_ops(void) {}
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index aacfcc90d929..2240f2b3643e 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -406,36 +406,6 @@ struct pci_eject_response {
static int pci_ring_size = (4 * PAGE_SIZE);
-struct hv_interrupt_entry {
- u32 source; /* 1 for MSI(-X) */
- u32 reserved1;
- u32 address;
- u32 data;
-};
-
-/*
- * flags for hv_device_interrupt_target.flags
- */
-#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
-#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
-
-struct hv_device_interrupt_target {
- u32 vector;
- u32 flags;
- union {
- u64 vp_mask;
- struct hv_vpset vp_set;
- };
-};
-
-struct retarget_msi_interrupt {
- u64 partition_id; /* use "self" */
- u64 device_id;
- struct hv_interrupt_entry int_entry;
- u64 reserved2;
- struct hv_device_interrupt_target int_target;
-} __packed __aligned(8);
-
/*
* Driver specific state.
*/
@@ -482,7 +452,7 @@ struct hv_pcibus_device {
struct workqueue_struct *wq;
/* hypercall arg, must not cross page boundary */
- struct retarget_msi_interrupt retarget_msi_interrupt_params;
+ struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
/*
* Don't put anything here: retarget_msi_interrupt_params must be last
@@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
{
struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
struct irq_cfg *cfg = irqd_cfg(data);
- struct retarget_msi_interrupt *params;
+ struct hv_retarget_device_interrupt *params;
struct hv_pcibus_device *hbus;
struct cpumask *dest;
cpumask_var_t tmp;
@@ -1200,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data)
memset(params, 0, sizeof(*params));
params->partition_id = HV_PARTITION_ID_SELF;
params->int_entry.source = 1; /* MSI(-X) */
- params->int_entry.address = msi_desc->msg.address_lo;
- params->int_entry.data = msi_desc->msg.data;
+ hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc);
+ params->int_entry.msi_entry.data = msi_desc->msg.data;
params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
(hbus->hdev->dev_instance.b[4] << 16) |
(hbus->hdev->dev_instance.b[7] << 8) |
--
2.24.1
^ permalink raw reply related
* RE: [PATCH V3] x86/Hyper-V: Balloon up according to request page number
From: Tianyu Lan @ 2020-01-21 3:36 UTC (permalink / raw)
To: Michael Kelley, lantianyu1986@gmail.com, KY Srinivasan,
Haiyang Zhang, Stephen Hemminger, sashal@kernel.org
Cc: linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
vkuznets, stable@vger.kernel.org
In-Reply-To: <MW2PR2101MB10529ECBC84A6BA130FB9134D7320@MW2PR2101MB1052.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Tuesday, January 21, 2020 5:18 AM
> To: lantianyu1986@gmail.com; KY Srinivasan <kys@microsoft.com>; Haiyang
> Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; sashal@kernel.org
> Cc: Tianyu Lan <Tianyu.Lan@microsoft.com>; linux-hyperv@vger.kernel.org;
> linux-kernel@vger.kernel.org; vkuznets <vkuznets@redhat.com>;
> stable@vger.kernel.org
> Subject: RE: [PATCH V3] x86/Hyper-V: Balloon up according to request page
> number
>
> From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Monday, January 20,
> 2020 12:42 AM
> >
> > Current code has assumption that balloon request memory size aligns
> > with 2MB. But actually Hyper-V doesn't guarantee such alignment. When
> > balloon driver receives non-aligned balloon request, it produces
> > warning and balloon up more memory than requested in order to keep 2MB
> alignment.
> > Remove the warning and balloon up memory according to actual requested
> > memory size.
> >
> > Fixes: f6712238471a ("hv: hv_balloon: avoid memory leak on alloc_error
> > of 2MB memory
> > block")
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > ---
> > Change since v2:
> > - Remove check between request page number and alloc_unit
> > in the alloc_balloon_pages() because it's redundant with
> > new change.
> > - Remove the "continue" just follwoing alloc_unit switch
> > from 2MB to 4K in order to avoid skipping allocated
> > memory.
> >
> > Change since v1:
> > - Change logic of switching alloc_unit from 2MB to 4KB
> > in the balloon_up() to avoid redundant iteration when
> > handle non-aligned page request.
> > - Remove 2MB alignment operation and comment in balloon_up()
> > ---
> > drivers/hv/hv_balloon.c | 17 ++++-------------
> > 1 file changed, 4 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c index
> > 7f3e7ab22d5d..73092a7a3345 100644
> > --- a/drivers/hv/hv_balloon.c
> > +++ b/drivers/hv/hv_balloon.c
> > @@ -1681,10 +1681,7 @@ static unsigned int alloc_balloon_pages(struct
> > hv_dynmem_device *dm,
> > unsigned int i, j;
> > struct page *pg;
> >
> > - if (num_pages < alloc_unit)
> > - return 0;
> > -
> > - for (i = 0; (i * alloc_unit) < num_pages; i++) {
> > + for (i = 0; i < num_pages / alloc_unit; i++) {
> > if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) >
> > HV_HYP_PAGE_SIZE)
> > return i * alloc_unit;
> > @@ -1722,7 +1719,7 @@ static unsigned int alloc_balloon_pages(struct
> > hv_dynmem_device *dm,
> >
> > }
> >
> > - return num_pages;
> > + return i * alloc_unit;
> > }
> >
> > static void balloon_up(union dm_msg_info *msg_info) @@ -1737,9
> > +1734,6 @@ static void balloon_up(union dm_msg_info *msg_info)
> > long avail_pages;
> > unsigned long floor;
> >
> > - /* The host balloons pages in 2M granularity. */
> > - WARN_ON_ONCE(num_pages % PAGES_IN_2M != 0);
> > -
> > /*
> > * We will attempt 2M allocations. However, if we fail to
> > * allocate 2M chunks, we will go back to PAGE_SIZE allocations.
> > @@ -1749,14 +1743,13 @@ static void balloon_up(union dm_msg_info
> *msg_info)
> > avail_pages = si_mem_available();
> > floor = compute_balloon_floor();
> >
> > - /* Refuse to balloon below the floor, keep the 2M granularity. */
> > + /* Refuse to balloon below the floor. */
> > if (avail_pages < num_pages || avail_pages - num_pages < floor) {
> > pr_warn("Balloon request will be partially fulfilled. %s\n",
> > avail_pages < num_pages ? "Not enough memory." :
> > "Balloon floor reached.");
> >
> > num_pages = avail_pages > floor ? (avail_pages - floor) : 0;
> > - num_pages -= num_pages % PAGES_IN_2M;
> > }
> >
> > while (!done) {
> > @@ -1770,10 +1763,8 @@ static void balloon_up(union dm_msg_info
> *msg_info)
> > num_ballooned = alloc_balloon_pages(&dm_device,
> num_pages,
> > bl_resp, alloc_unit);
> >
> > - if (alloc_unit != 1 && num_ballooned == 0) {
> > + if (alloc_unit != 1 && num_ballooned != num_pages)
> > alloc_unit = 1;
> > - continue;
> > - }
>
> I don't think removing the "continue" works either. Suppose the requested
> size is 1 Mbyte. With an alloc_unit of 2M, alloc_balloon_pages() will return
> zero. The code above will set alloc_unit to 1, which is correct. But without the
> "continue", the code below will mark "done" as true, and we won't loop back
> around to try again with alloc_unit set to 1.
>
> I think the original code needs to stay as is.
Yes, you are right. Will revert the change in the next version. Thanks.
>
> Michael
>
> >
> > if (num_ballooned == 0 || num_ballooned == num_pages) {
> > pr_debug("Ballooned %u out of %u requested
> pages.\n",
> > --
> > 2.14.5
^ permalink raw reply
* [PATCH V4] x86/Hyper-V: Balloon up according to request page number
From: lantianyu1986 @ 2020-01-21 3:49 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, vkuznets, stable
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
Current code has assumption that balloon request memory size aligns
with 2MB. But actually Hyper-V doesn't guarantee such alignment. When
balloon driver receives non-aligned balloon request, it produces warning
and balloon up more memory than requested in order to keep 2MB alignment.
Remove the warning and balloon up memory according to actual requested
memory size.
Fixes: f6712238471a ("hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory block")
Cc: stable@vger.kernel.org
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v3:
- Revert optimization of swtiching alloc_unit
Change since v2:
- Remove check between request page number and alloc_unit
in the alloc_balloon_pages() because it's redundant with
new change.
- Remove the "continue" just follwoing alloc_unit switch
from 2MB to 4K in order to avoid skipping allocated
memory.
Change since v1:
- Change logic of switching alloc_unit from 2MB to 4KB
in the balloon_up() to avoid redundant iteration when
handle non-aligned page request.
- Remove 2MB alignment operation and comment in balloon_up()
---
drivers/hv/hv_balloon.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 7f3e7ab22d5d..a03c5191101e 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1681,10 +1681,7 @@ static unsigned int alloc_balloon_pages(struct hv_dynmem_device *dm,
unsigned int i, j;
struct page *pg;
- if (num_pages < alloc_unit)
- return 0;
-
- for (i = 0; (i * alloc_unit) < num_pages; i++) {
+ for (i = 0; i < num_pages / alloc_unit; i++) {
if (bl_resp->hdr.size + sizeof(union dm_mem_page_range) >
HV_HYP_PAGE_SIZE)
return i * alloc_unit;
@@ -1722,7 +1719,7 @@ static unsigned int alloc_balloon_pages(struct hv_dynmem_device *dm,
}
- return num_pages;
+ return i * alloc_unit;
}
static void balloon_up(union dm_msg_info *msg_info)
@@ -1737,9 +1734,6 @@ static void balloon_up(union dm_msg_info *msg_info)
long avail_pages;
unsigned long floor;
- /* The host balloons pages in 2M granularity. */
- WARN_ON_ONCE(num_pages % PAGES_IN_2M != 0);
-
/*
* We will attempt 2M allocations. However, if we fail to
* allocate 2M chunks, we will go back to PAGE_SIZE allocations.
@@ -1749,14 +1743,13 @@ static void balloon_up(union dm_msg_info *msg_info)
avail_pages = si_mem_available();
floor = compute_balloon_floor();
- /* Refuse to balloon below the floor, keep the 2M granularity. */
+ /* Refuse to balloon below the floor. */
if (avail_pages < num_pages || avail_pages - num_pages < floor) {
pr_warn("Balloon request will be partially fulfilled. %s\n",
avail_pages < num_pages ? "Not enough memory." :
"Balloon floor reached.");
num_pages = avail_pages > floor ? (avail_pages - floor) : 0;
- num_pages -= num_pages % PAGES_IN_2M;
}
while (!done) {
--
2.14.5
^ permalink raw reply related
* RE: [PATCH V4] x86/Hyper-V: Balloon up according to request page number
From: Michael Kelley @ 2020-01-21 4:36 UTC (permalink / raw)
To: lantianyu1986@gmail.com, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org
Cc: Tianyu Lan, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, vkuznets, stable@vger.kernel.org
In-Reply-To: <20200121034912.2725-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com> Sent: Monday, January 20, 2020 7:49 PM
>
> Current code has assumption that balloon request memory size aligns
> with 2MB. But actually Hyper-V doesn't guarantee such alignment. When
> balloon driver receives non-aligned balloon request, it produces warning
> and balloon up more memory than requested in order to keep 2MB alignment.
> Remove the warning and balloon up memory according to actual requested
> memory size.
>
> Fixes: f6712238471a ("hv: hv_balloon: avoid memory leak on alloc_error of 2MB memory
> block")
> Cc: stable@vger.kernel.org
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefano Garzarella @ 2020-01-21 9:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, netdev, linux-kernel, Jorgen Hansen, Jason Wang,
kvm, Stefan Hajnoczi, virtualization, linux-hyperv, Dexuan Cui
In-Reply-To: <20200120170120-mutt-send-email-mst@kernel.org>
On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > >
> > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > network namespaces and could break existing applications.
> > > > > > >
> > > > > > > Sorry, no.
> > > > > > >
> > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > where these netns changes could break things.
> > > > > >
> > > > > > I forgot to mention the use case.
> > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > processes involved:
> > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > passes it to qemu
> > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > different netns, so the communication is not allowed
> > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > >
> > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > >
> > > > > David's question is what would be a usecase that's broken
> > > > > (as opposed to fixed) by enabling this by default.
> > > >
> > > > Yes, I got that. Thanks for clarifying.
> > > > I just reported a broken example that can be fixed with a different
> > > > design (due to the fact that before this series, vsock devices were
> > > > accessible to all netns).
> > > >
> > > > >
> > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > module parameter isn't that.
> > > >
> > > > Okay, but I honestly can't find a case that can't be solved.
> > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > a real case to come up.
> > > >
> > > > I'll try to see better if there's any particular case where we need
> > > > to disable netns in vsock.
> > > >
> > > > Thanks,
> > > > Stefano
> > >
> > > Me neither. so what did you have in mind when you wrote:
> > > "could break existing applications"?
> >
> > I had in mind:
> > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > older versions will not work with newer Linux.
>
> meaning they will keep not working, right?
Right, I mean without this series they work, with this series they work
only if the netns support is disabled or with a patch proposed but not
merged in kata.
>
> > 2. a single process running on init_netns that wants to communicate with
> > VMs handled by VMMs running in different netns, but this case can be
> > solved opening the /dev/vhost-vsock in the same netns of the process
> > that wants to communicate with the VMs (init_netns in this case), and
> > passig it to the VMM.
>
> again right now they just don't work, right?
Right, as above.
What do you recommend I do?
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH 2/2] pci: hyperv: Move retarget related struct definitions into tlfs
From: Vitaly Kuznetsov @ 2020-01-21 9:25 UTC (permalink / raw)
To: Boqun Feng
Cc: Michael Kelley, Boqun Feng, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
linux-hyperv, linux-kernel
In-Reply-To: <20200121015713.69691-2-boqun.feng@gmail.com>
Boqun Feng <boqun.feng@gmail.com> writes:
> For future support of virtual PCI on non-x86 architecture.
>
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 38 +++++++++++++++++++++++++++++
> arch/x86/include/asm/mshyperv.h | 8 ++++++
> drivers/pci/controller/pci-hyperv.c | 38 +++--------------------------
> 3 files changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index b9ebc20b2385..debe017ae748 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -912,4 +912,42 @@ struct hv_tlb_flush_ex {
> struct hv_partition_assist_pg {
> u32 tlb_lock_count;
> };
> +
> +union hv_msi_entry {
> + u64 as_uint64;
> + struct {
> + u32 address;
> + u32 data;
> + } __packed;
> +};
While Hyper-V code is full of this, I was once told that 'Union aliasing
is UB. Avoid it for good.' Maybe we should start getting rid of it
instead of adding more?
> +
> +struct hv_interrupt_entry {
> + u32 source; /* 1 for MSI(-X) */
> + u32 reserved1;
> + union hv_msi_entry msi_entry;
> +} __packed;
> +
> +/*
> + * flags for hv_device_interrupt_target.flags
> + */
> +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> +
> +struct hv_device_interrupt_target {
> + u32 vector;
> + u32 flags;
> + union {
> + u64 vp_mask;
> + struct hv_vpset vp_set;
> + };
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */
> +struct hv_retarget_device_interrupt {
> + u64 partition_id;
> + u64 device_id;
> + struct hv_interrupt_entry int_entry;
> + u64 reserved2;
> + struct hv_device_interrupt_target int_target;
> +} __packed __aligned(8);
> #endif
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 6b79515abb82..d13319d82f6b 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -240,6 +240,14 @@ bool hv_vcpu_is_preempted(int vcpu);
> static inline void hv_apic_init(void) {}
> #endif
>
> +#if IS_ENABLED(CONFIG_PCI_HYPERV)
> +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \
> +do { \
> + (msi_entry)->address = (msi_desc)->msg.address_lo; \
> +} while (0)
> +
> +#endif /* CONFIG_PCI_HYPERV */
It seems to be pointless to put defines under #if IS_ENABLED(): in case
it is not enabled and used you'll get a compilation error, in case it is
enabled and not used no code is going to be generated anyways.
> +
> #else /* CONFIG_HYPERV */
> static inline void hyperv_init(void) {}
> static inline void hyperv_setup_mmu_ops(void) {}
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index aacfcc90d929..2240f2b3643e 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -406,36 +406,6 @@ struct pci_eject_response {
>
> static int pci_ring_size = (4 * PAGE_SIZE);
>
> -struct hv_interrupt_entry {
> - u32 source; /* 1 for MSI(-X) */
> - u32 reserved1;
> - u32 address;
> - u32 data;
> -};
> -
> -/*
> - * flags for hv_device_interrupt_target.flags
> - */
> -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> -
> -struct hv_device_interrupt_target {
> - u32 vector;
> - u32 flags;
> - union {
> - u64 vp_mask;
> - struct hv_vpset vp_set;
> - };
> -};
> -
> -struct retarget_msi_interrupt {
> - u64 partition_id; /* use "self" */
> - u64 device_id;
> - struct hv_interrupt_entry int_entry;
> - u64 reserved2;
> - struct hv_device_interrupt_target int_target;
> -} __packed __aligned(8);
> -
> /*
> * Driver specific state.
> */
> @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> struct workqueue_struct *wq;
>
> /* hypercall arg, must not cross page boundary */
> - struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> /*
> * Don't put anything here: retarget_msi_interrupt_params must be last
> @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> {
> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt *params;
> + struct hv_retarget_device_interrupt *params;
> struct hv_pcibus_device *hbus;
> struct cpumask *dest;
> cpumask_var_t tmp;
> @@ -1200,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data)
> memset(params, 0, sizeof(*params));
> params->partition_id = HV_PARTITION_ID_SELF;
> params->int_entry.source = 1; /* MSI(-X) */
> - params->int_entry.address = msi_desc->msg.address_lo;
> - params->int_entry.data = msi_desc->msg.data;
> + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc);
I don't quite see why this hv_set_msi_address_from_desc() is needed at
all.
> + params->int_entry.msi_entry.data = msi_desc->msg.data;
> params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
--
Vitaly
^ permalink raw reply
* Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: David Miller @ 2020-01-21 10:04 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <1579558957-62496-2-git-send-email-haiyangz@microsoft.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Mon, 20 Jan 2020 14:22:36 -0800
> +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> + struct xdp_buff *xdp)
> +{
> + struct page *page = NULL;
> + void *data = nvchan->rsc.data[0];
> + u32 len = nvchan->rsc.len[0];
> + struct bpf_prog *prog;
> + u32 act = XDP_PASS;
Please use reverse christmas tree ordering of local variables.
> + xdp->data_hard_start = page_address(page);
> + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> + xdp_set_data_meta_invalid(xdp);
> + xdp->data_end = xdp->data + len;
> + xdp->rxq = &nvchan->xdp_rxq;
> + xdp->handle = 0;
> +
> + memcpy(xdp->data, data, len);
Why can't the program run directly on nvchan->rsc.data[0]?
This data copy defeats the whole performance gain of using XDP.
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Michael S. Tsirkin @ 2020-01-21 11:14 UTC (permalink / raw)
To: Stefano Garzarella
Cc: David Miller, netdev, linux-kernel, Jorgen Hansen, Jason Wang,
kvm, Stefan Hajnoczi, virtualization, linux-hyperv, Dexuan Cui
In-Reply-To: <CAGxU2F4uW7FNe5xC0sb3Xxr_GABSXuu1Z9n5M=Ntq==T7MaaVw@mail.gmail.com>
On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > >
> > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > network namespaces and could break existing applications.
> > > > > > > >
> > > > > > > > Sorry, no.
> > > > > > > >
> > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > where these netns changes could break things.
> > > > > > >
> > > > > > > I forgot to mention the use case.
> > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > processes involved:
> > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > passes it to qemu
> > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > different netns, so the communication is not allowed
> > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > >
> > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > >
> > > > > > David's question is what would be a usecase that's broken
> > > > > > (as opposed to fixed) by enabling this by default.
> > > > >
> > > > > Yes, I got that. Thanks for clarifying.
> > > > > I just reported a broken example that can be fixed with a different
> > > > > design (due to the fact that before this series, vsock devices were
> > > > > accessible to all netns).
> > > > >
> > > > > >
> > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > module parameter isn't that.
> > > > >
> > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > a real case to come up.
> > > > >
> > > > > I'll try to see better if there's any particular case where we need
> > > > > to disable netns in vsock.
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > >
> > > > Me neither. so what did you have in mind when you wrote:
> > > > "could break existing applications"?
> > >
> > > I had in mind:
> > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > older versions will not work with newer Linux.
> >
> > meaning they will keep not working, right?
>
> Right, I mean without this series they work, with this series they work
> only if the netns support is disabled or with a patch proposed but not
> merged in kata.
>
> >
> > > 2. a single process running on init_netns that wants to communicate with
> > > VMs handled by VMMs running in different netns, but this case can be
> > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > that wants to communicate with the VMs (init_netns in this case), and
> > > passig it to the VMM.
> >
> > again right now they just don't work, right?
>
> Right, as above.
>
> What do you recommend I do?
>
> Thanks,
> Stefano
If this breaks userspace, then we need to maintain compatibility.
For example, have two devices, /dev/vhost-vsock and /dev/vhost-vsock-netns?
--
MST
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefano Garzarella @ 2020-01-21 13:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, netdev, linux-kernel, Jorgen Hansen, Jason Wang,
kvm, Stefan Hajnoczi, virtualization, linux-hyperv, Dexuan Cui
In-Reply-To: <20200121055403-mutt-send-email-mst@kernel.org>
On Tue, Jan 21, 2020 at 06:14:48AM -0500, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > >
> > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > >
> > > > > > > > > Sorry, no.
> > > > > > > > >
> > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > where these netns changes could break things.
> > > > > > > >
> > > > > > > > I forgot to mention the use case.
> > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > processes involved:
> > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > passes it to qemu
> > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > different netns, so the communication is not allowed
> > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > >
> > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > >
> > > > > > > David's question is what would be a usecase that's broken
> > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > >
> > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > I just reported a broken example that can be fixed with a different
> > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > accessible to all netns).
> > > > > >
> > > > > > >
> > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > module parameter isn't that.
> > > > > >
> > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > a real case to come up.
> > > > > >
> > > > > > I'll try to see better if there's any particular case where we need
> > > > > > to disable netns in vsock.
> > > > > >
> > > > > > Thanks,
> > > > > > Stefano
> > > > >
> > > > > Me neither. so what did you have in mind when you wrote:
> > > > > "could break existing applications"?
> > > >
> > > > I had in mind:
> > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > older versions will not work with newer Linux.
> > >
> > > meaning they will keep not working, right?
> >
> > Right, I mean without this series they work, with this series they work
> > only if the netns support is disabled or with a patch proposed but not
> > merged in kata.
> >
> > >
> > > > 2. a single process running on init_netns that wants to communicate with
> > > > VMs handled by VMMs running in different netns, but this case can be
> > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > passig it to the VMM.
> > >
> > > again right now they just don't work, right?
> >
> > Right, as above.
> >
> > What do you recommend I do?
> >
> > Thanks,
> > Stefano
>
> If this breaks userspace, then we need to maintain compatibility.
> For example, have two devices, /dev/vhost-vsock and /dev/vhost-vsock-netns?
Interesting!
So, VMs handled with /dev/vhost-vsock will be reachable from any netns (as
it happens now) and VMs handled with /dev/vhost-vsock-netns will be
reachable only from the same netns of the process that opens it.
It requires more changes, but we will preserve the previous behavior,
adding the new feature!
Thanks a lot for this idea! I'll try to implement it!
Stefano
^ permalink raw reply
* RE: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-21 13:56 UTC (permalink / raw)
To: David Miller
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <20200121.110454.2077433904156411260.davem@davemloft.net>
> -----Original Message-----
> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Tuesday, January 21, 2020 5:05 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Mon, 20 Jan 2020 14:22:36 -0800
>
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> > + struct xdp_buff *xdp)
> > +{
> > + struct page *page = NULL;
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + struct bpf_prog *prog;
> > + u32 act = XDP_PASS;
>
> Please use reverse christmas tree ordering of local variables.
>
> > + xdp->data_hard_start = page_address(page);
> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> > + xdp_set_data_meta_invalid(xdp);
> > + xdp->data_end = xdp->data + len;
> > + xdp->rxq = &nvchan->xdp_rxq;
> > + xdp->handle = 0;
> > +
> > + memcpy(xdp->data, data, len);
>
> Why can't the program run directly on nvchan->rsc.data[0]?
>
> This data copy defeats the whole performance gain of using XDP.
Sure I will update this, and the var order.
Thanks
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefan Hajnoczi @ 2020-01-21 13:59 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Michael S. Tsirkin, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <CAGxU2F4uW7FNe5xC0sb3Xxr_GABSXuu1Z9n5M=Ntq==T7MaaVw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5589 bytes --]
On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > >
> > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > network namespaces and could break existing applications.
> > > > > > > >
> > > > > > > > Sorry, no.
> > > > > > > >
> > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > where these netns changes could break things.
> > > > > > >
> > > > > > > I forgot to mention the use case.
> > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > processes involved:
> > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > passes it to qemu
> > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > different netns, so the communication is not allowed
> > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > >
> > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > >
> > > > > > David's question is what would be a usecase that's broken
> > > > > > (as opposed to fixed) by enabling this by default.
> > > > >
> > > > > Yes, I got that. Thanks for clarifying.
> > > > > I just reported a broken example that can be fixed with a different
> > > > > design (due to the fact that before this series, vsock devices were
> > > > > accessible to all netns).
> > > > >
> > > > > >
> > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > module parameter isn't that.
> > > > >
> > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > a real case to come up.
> > > > >
> > > > > I'll try to see better if there's any particular case where we need
> > > > > to disable netns in vsock.
> > > > >
> > > > > Thanks,
> > > > > Stefano
> > > >
> > > > Me neither. so what did you have in mind when you wrote:
> > > > "could break existing applications"?
> > >
> > > I had in mind:
> > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > older versions will not work with newer Linux.
> >
> > meaning they will keep not working, right?
>
> Right, I mean without this series they work, with this series they work
> only if the netns support is disabled or with a patch proposed but not
> merged in kata.
>
> >
> > > 2. a single process running on init_netns that wants to communicate with
> > > VMs handled by VMMs running in different netns, but this case can be
> > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > that wants to communicate with the VMs (init_netns in this case), and
> > > passig it to the VMM.
> >
> > again right now they just don't work, right?
>
> Right, as above.
>
> What do you recommend I do?
Existing userspace applications must continue to work.
Guests are fine because G2H transports are always in the initial network
namespace.
On the host side we have a real case where Kata Containers and other
vsock users break. Existing applications run in other network
namespaces and assume they can communicate over vsock (it's only
available in the initial network namespace by default).
It seems we cannot isolate new network namespaces from the initial
network namespace by default because it will break existing
applications. That's a bummer.
There is one solution that maintains compatibility:
Introduce a per-namespace vsock isolation flag that can only transition
from false to true. Once it becomes true it cannot be reset to false
anymore (for security).
When vsock isolation is false the initial network namespace is used for
<CID, port> addressing.
When vsock isolation is true the current namespace is used for <CID,
port> addressing.
I guess the vsock isolation flag would be set via a rtnetlink message,
but I haven't checked.
The upshot is: existing software doesn't benefit from namespaces for
vsock isolation but it continues to work! New software makes 1 special
call after creating the namespace to opt in to vsock isolation.
This approach is secure because whoever sets up namespaces can
transition the flag from false to true and know that it can never be
reset to false anymore.
Does this make sense to everyone?
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Michael S. Tsirkin @ 2020-01-21 14:31 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefano Garzarella, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <20200121135907.GA641751@stefanha-x1.localdomain>
On Tue, Jan 21, 2020 at 01:59:07PM +0000, Stefan Hajnoczi wrote:
> On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > >
> > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > >
> > > > > > > > > Sorry, no.
> > > > > > > > >
> > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > where these netns changes could break things.
> > > > > > > >
> > > > > > > > I forgot to mention the use case.
> > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > processes involved:
> > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > passes it to qemu
> > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > different netns, so the communication is not allowed
> > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > >
> > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > >
> > > > > > > David's question is what would be a usecase that's broken
> > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > >
> > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > I just reported a broken example that can be fixed with a different
> > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > accessible to all netns).
> > > > > >
> > > > > > >
> > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > module parameter isn't that.
> > > > > >
> > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > a real case to come up.
> > > > > >
> > > > > > I'll try to see better if there's any particular case where we need
> > > > > > to disable netns in vsock.
> > > > > >
> > > > > > Thanks,
> > > > > > Stefano
> > > > >
> > > > > Me neither. so what did you have in mind when you wrote:
> > > > > "could break existing applications"?
> > > >
> > > > I had in mind:
> > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > older versions will not work with newer Linux.
> > >
> > > meaning they will keep not working, right?
> >
> > Right, I mean without this series they work, with this series they work
> > only if the netns support is disabled or with a patch proposed but not
> > merged in kata.
> >
> > >
> > > > 2. a single process running on init_netns that wants to communicate with
> > > > VMs handled by VMMs running in different netns, but this case can be
> > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > passig it to the VMM.
> > >
> > > again right now they just don't work, right?
> >
> > Right, as above.
> >
> > What do you recommend I do?
>
> Existing userspace applications must continue to work.
>
> Guests are fine because G2H transports are always in the initial network
> namespace.
>
> On the host side we have a real case where Kata Containers and other
> vsock users break. Existing applications run in other network
> namespaces and assume they can communicate over vsock (it's only
> available in the initial network namespace by default).
>
> It seems we cannot isolate new network namespaces from the initial
> network namespace by default because it will break existing
> applications. That's a bummer.
>
> There is one solution that maintains compatibility:
>
> Introduce a per-namespace vsock isolation flag that can only transition
> from false to true. Once it becomes true it cannot be reset to false
> anymore (for security).
>
> When vsock isolation is false the initial network namespace is used for
> <CID, port> addressing.
>
> When vsock isolation is true the current namespace is used for <CID,
> port> addressing.
>
> I guess the vsock isolation flag would be set via a rtnetlink message,
> but I haven't checked.
>
> The upshot is: existing software doesn't benefit from namespaces for
> vsock isolation but it continues to work! New software makes 1 special
> call after creating the namespace to opt in to vsock isolation.
>
> This approach is secure because whoever sets up namespaces can
> transition the flag from false to true and know that it can never be
> reset to false anymore.
>
> Does this make sense to everyone?
>
> Stefan
Anything wrong with a separate device? whoever opens it decides
whether netns will work ...
--
MST
^ permalink raw reply
* Re: [PATCH 2/2] pci: hyperv: Move retarget related struct definitions into tlfs
From: Bjorn Helgaas @ 2020-01-21 14:37 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-hyperv, linux-kernel, Michael Kelley, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS
In-Reply-To: <20200121015713.69691-2-boqun.feng@gmail.com>
On Tue, Jan 21, 2020 at 09:57:13AM +0800, Boqun Feng wrote:
> For future support of virtual PCI on non-x86 architecture.
1) Don't make up random subject line prefixes; look at previous
practice and follow it, e.g.,
$ git log --oneline drivers/pci/controller/pci-hyperv.c | head
877b911a5ba0 PCI: hv: Avoid a kmemleak false positive caused by the hbus buffer
14ef39fddd23 PCI: hv: Change pci_protocol_version to per-hbus
ac82fc832708 PCI: hv: Add hibernation support
a8e37506e79a PCI: hv: Reorganize the code in preparation of hibernation
f73f8a504e27 PCI: hv: Use bytes 4 and 5 from instance ID as the PCI domain numbers
348dd93e40c1 PCI: hv: Add a Hyper-V PCI interface driver for software backchannel interface
2) Make the commit log complete in itself. This one (and the previous
on) is not complete without reading the subject.
3) This patch claims to be a "move", but in fact it also *adds* union
hv_msi_entry, which didn't exist before. It's better if you do a pure
move that doesn't add or change things, plus a separate patch that
makes changes that need to be reviewed more closely.
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> ---
> arch/x86/include/asm/hyperv-tlfs.h | 38 +++++++++++++++++++++++++++++
> arch/x86/include/asm/mshyperv.h | 8 ++++++
> drivers/pci/controller/pci-hyperv.c | 38 +++--------------------------
> 3 files changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index b9ebc20b2385..debe017ae748 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -912,4 +912,42 @@ struct hv_tlb_flush_ex {
> struct hv_partition_assist_pg {
> u32 tlb_lock_count;
> };
> +
> +union hv_msi_entry {
> + u64 as_uint64;
> + struct {
> + u32 address;
> + u32 data;
> + } __packed;
> +};
> +
> +struct hv_interrupt_entry {
> + u32 source; /* 1 for MSI(-X) */
> + u32 reserved1;
> + union hv_msi_entry msi_entry;
> +} __packed;
> +
> +/*
> + * flags for hv_device_interrupt_target.flags
> + */
> +#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> +#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> +
> +struct hv_device_interrupt_target {
> + u32 vector;
> + u32 flags;
> + union {
> + u64 vp_mask;
> + struct hv_vpset vp_set;
> + };
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */
> +struct hv_retarget_device_interrupt {
> + u64 partition_id;
> + u64 device_id;
> + struct hv_interrupt_entry int_entry;
> + u64 reserved2;
> + struct hv_device_interrupt_target int_target;
> +} __packed __aligned(8);
> #endif
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 6b79515abb82..d13319d82f6b 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -240,6 +240,14 @@ bool hv_vcpu_is_preempted(int vcpu);
> static inline void hv_apic_init(void) {}
> #endif
>
> +#if IS_ENABLED(CONFIG_PCI_HYPERV)
> +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \
> +do { \
> + (msi_entry)->address = (msi_desc)->msg.address_lo; \
> +} while (0)
> +
> +#endif /* CONFIG_PCI_HYPERV */
> +
> #else /* CONFIG_HYPERV */
> static inline void hyperv_init(void) {}
> static inline void hyperv_setup_mmu_ops(void) {}
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index aacfcc90d929..2240f2b3643e 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -406,36 +406,6 @@ struct pci_eject_response {
>
> static int pci_ring_size = (4 * PAGE_SIZE);
>
> -struct hv_interrupt_entry {
> - u32 source; /* 1 for MSI(-X) */
> - u32 reserved1;
> - u32 address;
> - u32 data;
> -};
> -
> -/*
> - * flags for hv_device_interrupt_target.flags
> - */
> -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> -
> -struct hv_device_interrupt_target {
> - u32 vector;
> - u32 flags;
> - union {
> - u64 vp_mask;
> - struct hv_vpset vp_set;
> - };
> -};
> -
> -struct retarget_msi_interrupt {
> - u64 partition_id; /* use "self" */
> - u64 device_id;
> - struct hv_interrupt_entry int_entry;
> - u64 reserved2;
> - struct hv_device_interrupt_target int_target;
> -} __packed __aligned(8);
> -
> /*
> * Driver specific state.
> */
> @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> struct workqueue_struct *wq;
>
> /* hypercall arg, must not cross page boundary */
> - struct retarget_msi_interrupt retarget_msi_interrupt_params;
> + struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
>
> /*
> * Don't put anything here: retarget_msi_interrupt_params must be last
> @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> {
> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> struct irq_cfg *cfg = irqd_cfg(data);
> - struct retarget_msi_interrupt *params;
> + struct hv_retarget_device_interrupt *params;
> struct hv_pcibus_device *hbus;
> struct cpumask *dest;
> cpumask_var_t tmp;
> @@ -1200,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data)
> memset(params, 0, sizeof(*params));
> params->partition_id = HV_PARTITION_ID_SELF;
> params->int_entry.source = 1; /* MSI(-X) */
> - params->int_entry.address = msi_desc->msg.address_lo;
> - params->int_entry.data = msi_desc->msg.data;
> + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc);
> + params->int_entry.msi_entry.data = msi_desc->msg.data;
> params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> (hbus->hdev->dev_instance.b[4] << 16) |
> (hbus->hdev->dev_instance.b[7] << 8) |
> --
> 2.24.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefan Hajnoczi @ 2020-01-21 15:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Garzarella, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <20200121055403-mutt-send-email-mst@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 4855 bytes --]
On Tue, Jan 21, 2020 at 06:14:48AM -0500, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > >
> > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > >
> > > > > > > > > Sorry, no.
> > > > > > > > >
> > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > where these netns changes could break things.
> > > > > > > >
> > > > > > > > I forgot to mention the use case.
> > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > processes involved:
> > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > passes it to qemu
> > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > different netns, so the communication is not allowed
> > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > >
> > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > >
> > > > > > > David's question is what would be a usecase that's broken
> > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > >
> > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > I just reported a broken example that can be fixed with a different
> > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > accessible to all netns).
> > > > > >
> > > > > > >
> > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > module parameter isn't that.
> > > > > >
> > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > a real case to come up.
> > > > > >
> > > > > > I'll try to see better if there's any particular case where we need
> > > > > > to disable netns in vsock.
> > > > > >
> > > > > > Thanks,
> > > > > > Stefano
> > > > >
> > > > > Me neither. so what did you have in mind when you wrote:
> > > > > "could break existing applications"?
> > > >
> > > > I had in mind:
> > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > older versions will not work with newer Linux.
> > >
> > > meaning they will keep not working, right?
> >
> > Right, I mean without this series they work, with this series they work
> > only if the netns support is disabled or with a patch proposed but not
> > merged in kata.
> >
> > >
> > > > 2. a single process running on init_netns that wants to communicate with
> > > > VMs handled by VMMs running in different netns, but this case can be
> > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > passig it to the VMM.
> > >
> > > again right now they just don't work, right?
> >
> > Right, as above.
> >
> > What do you recommend I do?
> >
> > Thanks,
> > Stefano
>
> If this breaks userspace, then we need to maintain compatibility.
> For example, have two devices, /dev/vhost-vsock and /dev/vhost-vsock-netns?
/dev/vhost-vsock-netns is cleaner and simpler than my suggestion. I
like it!
This is nice for containers (say you want to run QEMU inside a container
on the host) because you can allow only /dev/vhost-vsock-netns inside
containers. This prevents them from opening /dev/vhost-vsock to get
access to the initial network namespace.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/3] vsock: add network namespace support
From: Stefan Hajnoczi @ 2020-01-21 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefano Garzarella, David Miller, netdev, linux-kernel,
Jorgen Hansen, Jason Wang, kvm, virtualization, linux-hyperv,
Dexuan Cui
In-Reply-To: <20200121093104-mutt-send-email-mst@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 6399 bytes --]
On Tue, Jan 21, 2020 at 09:31:42AM -0500, Michael S. Tsirkin wrote:
> On Tue, Jan 21, 2020 at 01:59:07PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Jan 21, 2020 at 10:07:06AM +0100, Stefano Garzarella wrote:
> > > On Mon, Jan 20, 2020 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Jan 20, 2020 at 05:53:39PM +0100, Stefano Garzarella wrote:
> > > > > On Mon, Jan 20, 2020 at 5:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > On Mon, Jan 20, 2020 at 02:58:01PM +0100, Stefano Garzarella wrote:
> > > > > > > On Mon, Jan 20, 2020 at 1:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > > On Mon, Jan 20, 2020 at 11:17:35AM +0100, Stefano Garzarella wrote:
> > > > > > > > > On Mon, Jan 20, 2020 at 10:06:10AM +0100, David Miller wrote:
> > > > > > > > > > From: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > > > > Date: Thu, 16 Jan 2020 18:24:26 +0100
> > > > > > > > > >
> > > > > > > > > > > This patch adds 'netns' module param to enable this new feature
> > > > > > > > > > > (disabled by default), because it changes vsock's behavior with
> > > > > > > > > > > network namespaces and could break existing applications.
> > > > > > > > > >
> > > > > > > > > > Sorry, no.
> > > > > > > > > >
> > > > > > > > > > I wonder if you can even design a legitimate, reasonable, use case
> > > > > > > > > > where these netns changes could break things.
> > > > > > > > >
> > > > > > > > > I forgot to mention the use case.
> > > > > > > > > I tried the RFC with Kata containers and we found that Kata shim-v1
> > > > > > > > > doesn't work (Kata shim-v2 works as is) because there are the following
> > > > > > > > > processes involved:
> > > > > > > > > - kata-runtime (runs in the init_netns) opens /dev/vhost-vsock and
> > > > > > > > > passes it to qemu
> > > > > > > > > - kata-shim (runs in a container) wants to talk with the guest but the
> > > > > > > > > vsock device is assigned to the init_netns and kata-shim runs in a
> > > > > > > > > different netns, so the communication is not allowed
> > > > > > > > > But, as you said, this could be a wrong design, indeed they already
> > > > > > > > > found a fix, but I was not sure if others could have the same issue.
> > > > > > > > >
> > > > > > > > > In this case, do you think it is acceptable to make this change in
> > > > > > > > > the vsock's behavior with netns and ask the user to change the design?
> > > > > > > >
> > > > > > > > David's question is what would be a usecase that's broken
> > > > > > > > (as opposed to fixed) by enabling this by default.
> > > > > > >
> > > > > > > Yes, I got that. Thanks for clarifying.
> > > > > > > I just reported a broken example that can be fixed with a different
> > > > > > > design (due to the fact that before this series, vsock devices were
> > > > > > > accessible to all netns).
> > > > > > >
> > > > > > > >
> > > > > > > > If it does exist, you need a way for userspace to opt-in,
> > > > > > > > module parameter isn't that.
> > > > > > >
> > > > > > > Okay, but I honestly can't find a case that can't be solved.
> > > > > > > So I don't know whether to add an option (ioctl, sysfs ?) or wait for
> > > > > > > a real case to come up.
> > > > > > >
> > > > > > > I'll try to see better if there's any particular case where we need
> > > > > > > to disable netns in vsock.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Stefano
> > > > > >
> > > > > > Me neither. so what did you have in mind when you wrote:
> > > > > > "could break existing applications"?
> > > > >
> > > > > I had in mind:
> > > > > 1. the Kata case. It is fixable (the fix is not merged on kata), but
> > > > > older versions will not work with newer Linux.
> > > >
> > > > meaning they will keep not working, right?
> > >
> > > Right, I mean without this series they work, with this series they work
> > > only if the netns support is disabled or with a patch proposed but not
> > > merged in kata.
> > >
> > > >
> > > > > 2. a single process running on init_netns that wants to communicate with
> > > > > VMs handled by VMMs running in different netns, but this case can be
> > > > > solved opening the /dev/vhost-vsock in the same netns of the process
> > > > > that wants to communicate with the VMs (init_netns in this case), and
> > > > > passig it to the VMM.
> > > >
> > > > again right now they just don't work, right?
> > >
> > > Right, as above.
> > >
> > > What do you recommend I do?
> >
> > Existing userspace applications must continue to work.
> >
> > Guests are fine because G2H transports are always in the initial network
> > namespace.
> >
> > On the host side we have a real case where Kata Containers and other
> > vsock users break. Existing applications run in other network
> > namespaces and assume they can communicate over vsock (it's only
> > available in the initial network namespace by default).
> >
> > It seems we cannot isolate new network namespaces from the initial
> > network namespace by default because it will break existing
> > applications. That's a bummer.
> >
> > There is one solution that maintains compatibility:
> >
> > Introduce a per-namespace vsock isolation flag that can only transition
> > from false to true. Once it becomes true it cannot be reset to false
> > anymore (for security).
> >
> > When vsock isolation is false the initial network namespace is used for
> > <CID, port> addressing.
> >
> > When vsock isolation is true the current namespace is used for <CID,
> > port> addressing.
> >
> > I guess the vsock isolation flag would be set via a rtnetlink message,
> > but I haven't checked.
> >
> > The upshot is: existing software doesn't benefit from namespaces for
> > vsock isolation but it continues to work! New software makes 1 special
> > call after creating the namespace to opt in to vsock isolation.
> >
> > This approach is secure because whoever sets up namespaces can
> > transition the flag from false to true and know that it can never be
> > reset to false anymore.
> >
> > Does this make sense to everyone?
> >
> > Stefan
>
> Anything wrong with a separate device? whoever opens it decides
> whether netns will work ...
Your idea is better. I think a separate device is the way to go.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Stefan Hajnoczi @ 2020-01-21 15:50 UTC (permalink / raw)
To: Stefano Garzarella
Cc: davem, netdev, linux-kernel, Jorgen Hansen, Jason Wang, kvm,
virtualization, linux-hyperv, Michael S. Tsirkin, Dexuan Cui
In-Reply-To: <20200116172428.311437-1-sgarzare@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 407 bytes --]
What should vsock_dev_do_ioctl() IOCTL_VM_SOCKETS_GET_LOCAL_CID return?
The answer is probably dependent on the caller's network namespace.
Ultimately we may need per-namespace transports. Imagine assigning a
G2H transport to a specific network namespace.
vsock_stream_connect() needs to be namespace-aware so that other
namespaces cannot use the G2H transport to send a connection
establishment packet.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* RE: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-21 18:53 UTC (permalink / raw)
To: David Miller
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <20200121.110454.2077433904156411260.davem@davemloft.net>
Sorry I was replying too quickly. See more detailed explanation below.
> -----Original Message-----
> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Tuesday, January 21, 2020 5:05 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Mon, 20 Jan 2020 14:22:36 -0800
>
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> > + struct xdp_buff *xdp)
> > +{
> > + struct page *page = NULL;
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + struct bpf_prog *prog;
> > + u32 act = XDP_PASS;
>
> Please use reverse christmas tree ordering of local variables.
Will do.
>
> > + xdp->data_hard_start = page_address(page);
> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> > + xdp_set_data_meta_invalid(xdp);
> > + xdp->data_end = xdp->data + len;
> > + xdp->rxq = &nvchan->xdp_rxq;
> > + xdp->handle = 0;
> > +
> > + memcpy(xdp->data, data, len);
>
> Why can't the program run directly on nvchan->rsc.data[0]?
The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
for XDP. We thought about re-use the RNDIS header space, but it's too
small. So we decided to copy the packets to a page buffer for XDP. And,
most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
most of the packets run on VF NIC. The synthetic NIC is considered as a
fallback data-path. So the data copy on netvsc won't impact performance
significantly.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: David Miller @ 2020-01-21 21:28 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <MN2PR21MB1375A6F208BC94CD4CFA016BCA0D0@MN2PR21MB1375.namprd21.prod.outlook.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Tue, 21 Jan 2020 18:53:28 +0000
> Sorry I was replying too quickly. See more detailed explanation below.
>
>> -----Original Message-----
>> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
>> owner@vger.kernel.org> On Behalf Of David Miller
>> Sent: Tuesday, January 21, 2020 5:05 AM
>> To: Haiyang Zhang <haiyangz@microsoft.com>
>> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
>> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
>> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
>> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>>
>> From: Haiyang Zhang <haiyangz@microsoft.com>
>> Date: Mon, 20 Jan 2020 14:22:36 -0800
>>
>> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
>> > + struct xdp_buff *xdp)
>> > +{
>> > + struct page *page = NULL;
>> > + void *data = nvchan->rsc.data[0];
>> > + u32 len = nvchan->rsc.len[0];
>> > + struct bpf_prog *prog;
>> > + u32 act = XDP_PASS;
>>
>> Please use reverse christmas tree ordering of local variables.
> Will do.
>
>>
>> > + xdp->data_hard_start = page_address(page);
>> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
>> > + xdp_set_data_meta_invalid(xdp);
>> > + xdp->data_end = xdp->data + len;
>> > + xdp->rxq = &nvchan->xdp_rxq;
>> > + xdp->handle = 0;
>> > +
>> > + memcpy(xdp->data, data, len);
>>
>> Why can't the program run directly on nvchan->rsc.data[0]?
>
> The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
> for XDP. We thought about re-use the RNDIS header space, but it's too
> small. So we decided to copy the packets to a page buffer for XDP. And,
> most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
> most of the packets run on VF NIC. The synthetic NIC is considered as a
> fallback data-path. So the data copy on netvsc won't impact performance
> significantly.
You need to explain this in your commit message otherwise every reviewer with
XDP expertiece will ask the same question.
^ permalink raw reply
* RE: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 2:12 UTC (permalink / raw)
To: David Miller
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <20200121.222829.888926574980511328.davem@davemloft.net>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> owner@vger.kernel.org> On Behalf Of David Miller
> Sent: Tuesday, January 21, 2020 4:28 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Tue, 21 Jan 2020 18:53:28 +0000
>
> > Sorry I was replying too quickly. See more detailed explanation below.
> >
> >> -----Original Message-----
> >> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> >> owner@vger.kernel.org> On Behalf Of David Miller
> >> Sent: Tuesday, January 21, 2020 5:05 AM
> >> To: Haiyang Zhang <haiyangz@microsoft.com>
> >> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> >> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> >> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> >> <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH V2,net-next, 1/2] hv_netvsc: Add XDP support
> >>
> >> From: Haiyang Zhang <haiyangz@microsoft.com>
> >> Date: Mon, 20 Jan 2020 14:22:36 -0800
> >>
> >> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel
> *nvchan,
> >> > + struct xdp_buff *xdp)
> >> > +{
> >> > + struct page *page = NULL;
> >> > + void *data = nvchan->rsc.data[0];
> >> > + u32 len = nvchan->rsc.len[0];
> >> > + struct bpf_prog *prog;
> >> > + u32 act = XDP_PASS;
> >>
> >> Please use reverse christmas tree ordering of local variables.
> > Will do.
> >
> >>
> >> > + xdp->data_hard_start = page_address(page);
> >> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> >> > + xdp_set_data_meta_invalid(xdp);
> >> > + xdp->data_end = xdp->data + len;
> >> > + xdp->rxq = &nvchan->xdp_rxq;
> >> > + xdp->handle = 0;
> >> > +
> >> > + memcpy(xdp->data, data, len);
> >>
> >> Why can't the program run directly on nvchan->rsc.data[0]?
> >
> > The Azure/Hyper-V synthetic NIC receive buffer doesn't provide
> > headroom for XDP. We thought about re-use the RNDIS header space, but
> > it's too small. So we decided to copy the packets to a page buffer for
> > XDP. And, most of our VMs on Azure have Accelerated Network (SRIOV)
> > enabled, so most of the packets run on VF NIC. The synthetic NIC is
> > considered as a fallback data-path. So the data copy on netvsc won't
> > impact performance significantly.
>
> You need to explain this in your commit message otherwise every reviewer
> with XDP expertiece will ask the same question.
Will do.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next 0/3] vsock: support network namespace
From: Stefano Garzarella @ 2020-01-22 9:13 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: davem, netdev, linux-kernel, Jorgen Hansen, Jason Wang, kvm,
virtualization, linux-hyperv, Michael S. Tsirkin, Dexuan Cui
In-Reply-To: <20200121155053.GD641751@stefanha-x1.localdomain>
On Tue, Jan 21, 2020 at 03:50:53PM +0000, Stefan Hajnoczi wrote:
> What should vsock_dev_do_ioctl() IOCTL_VM_SOCKETS_GET_LOCAL_CID return?
> The answer is probably dependent on the caller's network namespace.
Right, and I'm not handling this case. I'll fix!
>
> Ultimately we may need per-namespace transports. Imagine assigning a
> G2H transport to a specific network namespace.
Agree.
>
> vsock_stream_connect() needs to be namespace-aware so that other
> namespaces cannot use the G2H transport to send a connection
> establishment packet.
Right, maybe I can change the vsock_assign_transport() to check if a
transport can be assigned to a socket, checking the namespace.
I'll send a v2 handling these cases and implementing the Michael's idea
about /dev/vhost-vsock-netns
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH v2 1/4] Tools: hv: Reopen the devices if read() or write() returns errors
From: Michael Kelley @ 2020-01-22 16:53 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB01486C8C746F8936450C4CE1BF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:30 PM
>
> The state machine in the hv_utils driver can run out of order in some
> corner cases, e.g. if the kvp daemon doesn't call write() fast enough
> due to some reason, kvp_timeout_func() can run first and move the state
> to HVUTIL_READY; next, when kvp_on_msg() is called it returns -EINVAL
> since kvp_transaction.state is smaller than HVUTIL_USERSPACE_REQ; later,
> the daemon's write() gets an error -EINVAL, and the daemon will exit().
>
> We can reproduce the issue by sending a SIGSTOP signal to the daemon, wait
> for 1 minute, and send a SIGCONT signal to the daemon: the daemon will
> exit() quickly.
>
> We can fix the issue by forcing a reset of the device (which means the
> daemon can close() and open() the device again) and doing extra necessary
> clean-up.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> tools/hv/hv_fcopy_daemon.c | 19 +++++++++++++++----
> tools/hv/hv_kvp_daemon.c | 25 ++++++++++++++-----------
> tools/hv/hv_vss_daemon.c | 25 +++++++++++++++++++------
> 3 files changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
> index aea2d91ab364..a78a5292589b 100644
> --- a/tools/hv/hv_fcopy_daemon.c
> +++ b/tools/hv/hv_fcopy_daemon.c
> @@ -21,7 +21,7 @@
> #include <fcntl.h>
> #include <getopt.h>
>
> -static int target_fd;
> +static int target_fd = -1;
> static char target_fname[PATH_MAX];
> static unsigned long long filesize;
>
> @@ -80,6 +80,8 @@ static int hv_start_fcopy(struct hv_start_fcopy *smsg)
>
> error = 0;
> done:
> + if (error)
> + memset(target_fname, 0, sizeof(target_fname));
> return error;
> }
>
> @@ -111,12 +113,16 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
> static int hv_copy_finished(void)
> {
> close(target_fd);
> + target_fd = -1;
> + memset(target_fname, 0, sizeof(target_fname));
I'm not completely clear on why target_fd and target_fname need to
be reset. Could you add a comment with an explanation? Also,
since target_fname is a null terminated string, it seems like
target_fname[0] = 0 would be sufficient vs. zero'ing all 4096 bytes
(PATH_MAX).
> return 0;
> }
> static int hv_copy_cancel(void)
> {
> close(target_fd);
> + target_fd = -1;
> unlink(target_fname);
> + memset(target_fname, 0, sizeof(target_fname));
> return 0;
>
> }
> @@ -141,7 +147,7 @@ int main(int argc, char *argv[])
> struct hv_do_fcopy copy;
> __u32 kernel_modver;
> } buffer = { };
> - int in_handshake = 1;
> + int in_handshake;
>
> static struct option long_options[] = {
> {"help", no_argument, 0, 'h' },
> @@ -170,6 +176,9 @@ int main(int argc, char *argv[])
> openlog("HV_FCOPY", 0, LOG_USER);
> syslog(LOG_INFO, "starting; pid is:%d", getpid());
>
> +reopen_fcopy_fd:
> + hv_copy_cancel();
> + in_handshake = 1;
> fcopy_fd = open("/dev/vmbus/hv_fcopy", O_RDWR);
>
> if (fcopy_fd < 0) {
> @@ -196,7 +205,8 @@ int main(int argc, char *argv[])
> len = pread(fcopy_fd, &buffer, sizeof(buffer), 0);
> if (len < 0) {
> syslog(LOG_ERR, "pread failed: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> + close(fcopy_fd);
> + goto reopen_fcopy_fd;
In this case and all similar cases in this patch, there may be some risk
of getting stuck in a tight loop doing reopens if things are broken
in some strange and bizarre way. Having an absolute limit on the
number of reopens is potentially too restrictive as it could limit the
number of times a VM could be hibernated and resumed. Ideally
there could a simple rate limit on the reopens -- if it happens too frequently,
go ahead and exit like the current code does. Thoughts?
> }
>
> if (in_handshake) {
> @@ -233,7 +243,8 @@ int main(int argc, char *argv[])
>
> if (pwrite(fcopy_fd, &error, sizeof(int), 0) != sizeof(int)) {
> syslog(LOG_ERR, "pwrite failed: %s", strerror(errno));
> - exit(EXIT_FAILURE);
> + close(fcopy_fd);
> + goto reopen_fcopy_fd;
> }
> }
> }
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index e9ef4ca6a655..3282d48c4487 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -76,7 +76,7 @@ enum {
> DNS
> };
>
> -static int in_hand_shake = 1;
> +static int in_hand_shake;
>
> static char *os_name = "";
> static char *os_major = "";
> @@ -1400,14 +1400,6 @@ int main(int argc, char *argv[])
> openlog("KVP", 0, LOG_USER);
> syslog(LOG_INFO, "KVP starting; pid is:%d", getpid());
>
> - kvp_fd = open("/dev/vmbus/hv_kvp", O_RDWR | O_CLOEXEC);
> -
> - if (kvp_fd < 0) {
> - syslog(LOG_ERR, "open /dev/vmbus/hv_kvp failed; error: %d %s",
> - errno, strerror(errno));
> - exit(EXIT_FAILURE);
> - }
> -
> /*
> * Retrieve OS release information.
> */
> @@ -1423,6 +1415,16 @@ int main(int argc, char *argv[])
> exit(EXIT_FAILURE);
> }
>
> +reopen_kvp_fd:
> + in_hand_shake = 1;
> + kvp_fd = open("/dev/vmbus/hv_kvp", O_RDWR | O_CLOEXEC);
> +
> + if (kvp_fd < 0) {
> + syslog(LOG_ERR, "open /dev/vmbus/hv_kvp failed; error: %d %s",
> + errno, strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> /*
> * Register ourselves with the kernel.
> */
> @@ -1458,7 +1460,7 @@ int main(int argc, char *argv[])
> errno, strerror(errno));
>
> close(kvp_fd);
> - return EXIT_FAILURE;
> + goto reopen_kvp_fd;
> }
>
> /*
> @@ -1623,7 +1625,8 @@ int main(int argc, char *argv[])
> if (len != sizeof(struct hv_kvp_msg)) {
> syslog(LOG_ERR, "write failed; error: %d %s", errno,
> strerror(errno));
> - exit(EXIT_FAILURE);
> + close(kvp_fd);
> + goto reopen_kvp_fd;
> }
> }
>
> diff --git a/tools/hv/hv_vss_daemon.c b/tools/hv/hv_vss_daemon.c
> index 92902a88f671..e70fed66a5ae 100644
> --- a/tools/hv/hv_vss_daemon.c
> +++ b/tools/hv/hv_vss_daemon.c
> @@ -28,6 +28,8 @@
> #include <stdbool.h>
> #include <dirent.h>
>
> +static bool fs_frozen;
> +
> /* Don't use syslog() in the function since that can cause write to disk */
> static int vss_do_freeze(char *dir, unsigned int cmd)
> {
> @@ -155,8 +157,11 @@ static int vss_operate(int operation)
> continue;
> }
> error |= vss_do_freeze(ent->mnt_dir, cmd);
> - if (error && operation == VSS_OP_FREEZE)
> - goto err;
> + if (operation == VSS_OP_FREEZE) {
> + if (error)
> + goto err;
> + fs_frozen = true;
> + }
> }
>
> endmntent(mounts);
Shortly after the above code, there's code specifically to
do the root filesystem last. It has the same error test as above,
and it seems like it should also be setting fs_frozen = true if
it is successful.
> @@ -167,6 +172,9 @@ static int vss_operate(int operation)
> goto err;
> }
>
> + if (operation == VSS_OP_THAW && !error)
> + fs_frozen = false;
> +
> goto out;
> err:
> save_errno = errno;
> @@ -175,6 +183,7 @@ static int vss_operate(int operation)
> endmntent(mounts);
> }
> vss_operate(VSS_OP_THAW);
> + fs_frozen = false;
> /* Call syslog after we thaw all filesystems */
> if (ent)
> syslog(LOG_ERR, "FREEZE of %s failed; error:%d %s",
> @@ -202,7 +211,7 @@ int main(int argc, char *argv[])
> int op;
> struct hv_vss_msg vss_msg[1];
> int daemonize = 1, long_index = 0, opt;
> - int in_handshake = 1;
> + int in_handshake;
> __u32 kernel_modver;
>
> static struct option long_options[] = {
> @@ -232,6 +241,10 @@ int main(int argc, char *argv[])
> openlog("Hyper-V VSS", 0, LOG_USER);
> syslog(LOG_INFO, "VSS starting; pid is:%d", getpid());
>
> +reopen_vss_fd:
> + if (fs_frozen)
> + vss_operate(VSS_OP_THAW);
Need to set fs_frozen = false after the above statement?
> + in_handshake = 1;
> vss_fd = open("/dev/vmbus/hv_vss", O_RDWR);
> if (vss_fd < 0) {
> syslog(LOG_ERR, "open /dev/vmbus/hv_vss failed; error: %d %s",
> @@ -285,7 +298,7 @@ int main(int argc, char *argv[])
> syslog(LOG_ERR, "read failed; error:%d %s",
> errno, strerror(errno));
> close(vss_fd);
> - return EXIT_FAILURE;
> + goto reopen_vss_fd;
> }
>
> op = vss_msg->vss_hdr.operation;
> @@ -318,8 +331,8 @@ int main(int argc, char *argv[])
> syslog(LOG_ERR, "write failed; error: %d %s", errno,
> strerror(errno));
>
> - if (op == VSS_OP_FREEZE)
> - vss_operate(VSS_OP_THAW);
> + close(vss_fd);
> + goto reopen_vss_fd;
> }
> }
>
> --
> 2.19.1
^ permalink raw reply
* RE: [PATCH v2 2/4] hv_utils: Support host-initiated restart request
From: Michael Kelley @ 2020-01-22 17:16 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB01487F54363A856730BB13FDBF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:31 PM
>
>
> To test the code, run this command on the host:
>
> Restart-VM $vm -Type Reboot
>
Need a better commit message here. How about:
The hv_util driver currently supports a "shutdown" operation initiated from the
Hyper-V host. Newer versions of Hyper-V also support a "restart" operation. So
add support for the updated protocol version that has "restart" support, and
perform a clean reboot when such a message is received from Hyper-V.
To test the restart functionality, run this PowerShell command on the Hyper-V host:
Restart-VM <vmname> -Type Reboot
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_util.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 766bd8457346..fe3a316380c2 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -24,6 +24,8 @@
>
> #define SD_MAJOR 3
> #define SD_MINOR 0
> +#define SD_MINOR_1 1
> +#define SD_VERSION_3_1 (SD_MAJOR << 16 | SD_MINOR_1)
> #define SD_VERSION (SD_MAJOR << 16 | SD_MINOR)
>
> #define SD_MAJOR_1 1
> @@ -50,8 +52,9 @@ static int sd_srv_version;
> static int ts_srv_version;
> static int hb_srv_version;
>
> -#define SD_VER_COUNT 2
> +#define SD_VER_COUNT 3
> static const int sd_versions[] = {
> + SD_VERSION_3_1,
> SD_VERSION,
> SD_VERSION_1
> };
> @@ -118,11 +121,21 @@ static void perform_shutdown(struct work_struct *dummy)
> orderly_poweroff(true);
> }
>
> +static void perform_restart(struct work_struct *dummy)
> +{
> + orderly_reboot();
> +}
> +
> /*
> * Perform the shutdown operation in a thread context.
> */
> static DECLARE_WORK(shutdown_work, perform_shutdown);
>
> +/*
> + * Perform the restart operation in a thread context.
> + */
> +static DECLARE_WORK(restart_work, perform_restart);
> +
> static void shutdown_onchannelcallback(void *context)
> {
> struct vmbus_channel *channel = context;
> @@ -166,6 +179,14 @@ static void shutdown_onchannelcallback(void *context)
> pr_info("Shutdown request received -"
> " graceful shutdown initiated\n");
> break;
> + case 2:
> + case 3:
How are the flags values 0, 1, 2, and 3 interpreted? Perhaps a short comment
would be helpful.
> + pr_info("Restart request received -"
> + " graceful restart initiated\n");
> + icmsghdrp->status = HV_S_OK;
> +
> + schedule_work(&restart_work);
> + break;
For case 0 and 1 (shutdown), the schedule_work() call is performed only
after the response packet has been sent to the host. Is there a reason the
new code for case 2 and 3 (restart) is doing it in the opposite order?
> default:
> icmsghdrp->status = HV_E_FAIL;
> execute_shutdown = false;
> --
> 2.19.1
^ permalink raw reply
* [PATCH V3,net-next, 0/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
Add XDP support and update related document.
Haiyang Zhang (2):
hv_netvsc: Add XDP support
hv_netvsc: Update document for XDP support
.../networking/device_drivers/microsoft/netvsc.txt | 21 +++
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 21 ++-
drivers/net/hyperv/netvsc.c | 31 ++-
drivers/net/hyperv/netvsc_bpf.c | 209 +++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 175 ++++++++++++++---
drivers/net/hyperv/rndis_filter.c | 2 +-
7 files changed, 422 insertions(+), 39 deletions(-)
create mode 100644 drivers/net/hyperv/netvsc_bpf.c
--
1.8.3.1
^ permalink raw reply
* [PATCH V3,net-next, 2/2] hv_netvsc: Update document for XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579713814-36061-1-git-send-email-haiyangz@microsoft.com>
Added the new section in the document regarding XDP support
by hv_netvsc driver.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
.../networking/device_drivers/microsoft/netvsc.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/networking/device_drivers/microsoft/netvsc.txt b/Documentation/networking/device_drivers/microsoft/netvsc.txt
index 3bfa635..cd63556 100644
--- a/Documentation/networking/device_drivers/microsoft/netvsc.txt
+++ b/Documentation/networking/device_drivers/microsoft/netvsc.txt
@@ -82,3 +82,24 @@ Features
contain one or more packets. The send buffer is an optimization, the driver
will use slower method to handle very large packets or if the send buffer
area is exhausted.
+
+ XDP support
+ -----------
+ XDP (eXpress Data Path) is a feature that runs eBPF bytecode at the early
+ stage when packets arrive at a NIC card. The goal is to increase performance
+ for packet processing, reducing the overhead of SKB allocation and other
+ upper network layers.
+
+ hv_netvsc supports XDP in native mode, and transparently sets the XDP
+ program on the associated VF NIC as well.
+
+ Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
+ VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
+ is not recommended, also not propagated to synthetic NIC, and may be
+ overwritten by setting of synthetic NIC.
+
+ XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
+ before running XDP:
+ ethtool -K eth0 lro off
+
+ XDP_REDIRECT action is not yet supported.
--
1.8.3.1
^ permalink raw reply related
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