* [PATCH net-next 1/3] net: core: add helper tcp_v6_gso_csum_prep
From: Heiner Kallweit @ 2020-02-17 21:40 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers, Jay Cliburn,
Chris Snook, Rasesh Mody, Sudarsana Kalluru, GR-Linux-NIC-Dev,
Christian Benvenuti, Govindarajulu Varadarajan, Parvi Kaustubhi,
Jeff Kirsher, Guo-Fu Tseng, Shannon Nelson, Pensando Drivers,
Timur Tabi, Jassi Brar, Ilias Apalodimas, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Sasha Levin, Ronak Doshi,
VMware, Inc.
Cc: netdev@vger.kernel.org, Linux Kernel Mailing List,
intel-wired-lan, linux-hyperv, Linux USB Mailing List
In-Reply-To: <76cd6cfc-f4f3-ece7-203a-0266b7f02a12@gmail.com>
Several network drivers for chips that support TSO6 share the same code
for preparing the TCP header. A difference is that some reset the
payload_len whilst others don't do this. Let's factor out this common
code to a new helper.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
include/net/ip6_checksum.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/include/net/ip6_checksum.h b/include/net/ip6_checksum.h
index 7bec95df4..ef0130023 100644
--- a/include/net/ip6_checksum.h
+++ b/include/net/ip6_checksum.h
@@ -76,6 +76,18 @@ static inline void __tcp_v6_send_check(struct sk_buff *skb,
}
}
+static inline void tcp_v6_gso_csum_prep(struct sk_buff *skb,
+ bool clear_payload_len)
+{
+ struct ipv6hdr *ipv6h = ipv6_hdr(skb);
+ struct tcphdr *th = tcp_hdr(skb);
+
+ if (clear_payload_len)
+ ipv6h->payload_len = 0;
+
+ th->check = ~tcp_v6_check(0, &ipv6h->saddr, &ipv6h->daddr, 0);
+}
+
#if IS_ENABLED(CONFIG_IPV6)
static inline void tcp_v6_send_check(struct sock *sk, struct sk_buff *skb)
{
--
2.25.0
^ permalink raw reply related
* [PATCH net-next 0/3] net: core: add helper tcp_v6_gso_csum_prep
From: Heiner Kallweit @ 2020-02-17 21:39 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers, Jay Cliburn,
Chris Snook, Rasesh Mody, Sudarsana Kalluru, GR-Linux-NIC-Dev,
Christian Benvenuti, Govindarajulu Varadarajan, Parvi Kaustubhi,
Jeff Kirsher, Guo-Fu Tseng, Shannon Nelson, Pensando Drivers,
Timur Tabi, Jassi Brar, Ilias Apalodimas, K. Y. Srinivasan,
Haiyang Zhang, Stephen Hemminger, Sasha Levin, Ronak Doshi,
VMware, Inc.
Cc: netdev@vger.kernel.org, Linux Kernel Mailing List,
intel-wired-lan, linux-hyperv, Linux USB Mailing List
Several network drivers for chips that support TSO6 share the same code
for preparing the TCP header. A difference is that some reset the
payload_len whilst others don't do this. Let's factor out this common
code to a new helper.
Heiner Kallweit (3):
net: core: add helper tcp_v6_gso_csum_prep
r8169: use new helper tcp_v6_gso_csum_prep
net: use new helper tcp_v6_gso_csum_prep
drivers/net/ethernet/atheros/alx/main.c | 5 +---
.../net/ethernet/atheros/atl1c/atl1c_main.c | 6 ++---
drivers/net/ethernet/brocade/bna/bnad.c | 7 +----
drivers/net/ethernet/cisco/enic/enic_main.c | 3 +--
drivers/net/ethernet/intel/e1000/e1000_main.c | 6 +----
drivers/net/ethernet/intel/e1000e/netdev.c | 5 +---
drivers/net/ethernet/jme.c | 7 +----
.../net/ethernet/pensando/ionic/ionic_txrx.c | 5 +---
drivers/net/ethernet/qualcomm/emac/emac-mac.c | 7 ++---
drivers/net/ethernet/realtek/r8169_main.c | 26 ++-----------------
drivers/net/ethernet/socionext/netsec.c | 6 +----
drivers/net/hyperv/netvsc_drv.c | 5 +---
drivers/net/usb/r8152.c | 26 ++-----------------
drivers/net/vmxnet3/vmxnet3_drv.c | 5 +---
include/net/ip6_checksum.h | 12 +++++++++
15 files changed, 30 insertions(+), 101 deletions(-)
--
2.25.0
^ permalink raw reply
* RE: Is it safe for a NIC driver to use all the 48 bytes of skb->cb?
From: Dexuan Cui @ 2020-02-15 18:04 UTC (permalink / raw)
To: Haiyang Zhang, Stephen Hemminger, David S. Miller,
netdev@vger.kernel.org, KY Srinivasan,
linux-kernel@vger.kernel.org
Cc: linux-hyperv@vger.kernel.org
In-Reply-To: <MN2PR21MB1437345219FA1CC3A75B9875CA140@MN2PR21MB1437.namprd21.prod.outlook.com>
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Sent: Saturday, February 15, 2020 7:20 AM
> To: Dexuan Cui <decui@microsoft.com>; Stephen Hemminger
>
> According to the comments in skbuff.h below, it is the responsibility of the
> owning layer to make a SKB clone, if it wants to keep the data across layers.
> So, every layer can still use all of the 48 bytes.
>
> /*
> * This is the control buffer. It is free to use for every
> * layer. Please put your private variables there. If you
> * want to keep them across layers you have to do a skb_clone()
> * first. This is owned by whoever has the skb queued ATM.
> */
> char cb[48] __aligned(8);
>
> > Now hv_netvsc assumes it can use all of the 48-bytes, though it uses only
> > 20 bytes, but just in case the struct hv_netvsc_packet grows to >32 bytes in
> the
> > future, should we change the BUILD_BUG_ON() in netvsc_start_xmit() to
> > BUILD_BUG_ON(sizeof(struct hv_netvsc_packet) > SKB_SGO_CB_OFFSET); ?
>
> Based on the explanation above, the existing hv_netvsc code is correct.
>
> Thanks,
> - Haiyang
Got it. So if the upper layer saves something in the cb, it must do a skb_clone()
and pass the new skb to hv_netvsc. hv_netvsc is the lowest layer in the network
stack, so it can use all the 48 bytes without calling skb_clone().
BTW, now I happen to have a different question: in netvsc_probe() we have
net->needed_headroom = RNDIS_AND_PPI_SIZE;
I think this means when the network stack (ARP, IP, ICMP, TCP, UDP,etc) passes a
skb to hv_netvsc, the skb's headroom is increased by an extra size of
net->needed_headroom, right? Then in netvsc_xmit(), why do we still need to
call skb_cow_head(skb, RNDIS_AND_PPI_SIZE)? -- this looks unnecessary to me?
PS, what does the "cow" here mean? Copy On Write? It looks skb_cow_head()
just copies the data (if necessary) and it has nothing to do with the
write-protection in the MMU code.
Thanks,
Dexuan
^ permalink raw reply
* RE: Is it safe for a NIC driver to use all the 48 bytes of skb->cb?
From: Haiyang Zhang @ 2020-02-15 15:20 UTC (permalink / raw)
To: Dexuan Cui, Stephen Hemminger, David S. Miller,
netdev@vger.kernel.org, KY Srinivasan,
linux-kernel@vger.kernel.org
Cc: linux-hyperv@vger.kernel.org
In-Reply-To: <HK0P153MB0148311C48144413792A0FBEBF140@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
> -----Original Message-----
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Saturday, February 15, 2020 12:24 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; David S. Miller <davem@davemloft.net>;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; linux-
> kernel@vger.kernel.org
> Cc: linux-hyperv@vger.kernel.org
> Subject: Is it safe for a NIC driver to use all the 48 bytes of skb->cb?
>
> Hi,
> It looks all the layers of drivers among the network stack can use the 48-byte
> skb->cb array. Is there any rule how they should coordinate with each other?
>
> I noticed the last 16 bytes are used by struct skb_gso_cb:
>
> include/linux/skbuff.h:
> struct skb_gso_cb {
> union {
> int mac_offset;
> int data_offset;
> };
> int encap_level;
> __wsum csum;
> __u16 csum_start;
> };
> #define SKB_SGO_CB_OFFSET 32
> #define SKB_GSO_CB(skb) ((struct skb_gso_cb *)((skb)->cb +
> SKB_SGO_CB_OFFSET))
>
> Does this mean a low level NIC driver (e.g. hv_netvsc) should only use the first
> 32 bytes? What if the upper layer network stack starts to take up more space in
> the future?
According to the comments in skbuff.h below, it is the responsibility of the owning
layer to make a SKB clone, if it wants to keep the data across layers. So, every layer
can still use all of the 48 bytes.
/*
* This is the control buffer. It is free to use for every
* layer. Please put your private variables there. If you
* want to keep them across layers you have to do a skb_clone()
* first. This is owned by whoever has the skb queued ATM.
*/
char cb[48] __aligned(8);
> Now hv_netvsc assumes it can use all of the 48-bytes, though it uses only
> 20 bytes, but just in case the struct hv_netvsc_packet grows to >32 bytes in the
> future, should we change the BUILD_BUG_ON() in netvsc_start_xmit() to
> BUILD_BUG_ON(sizeof(struct hv_netvsc_packet) > SKB_SGO_CB_OFFSET); ?
Based on the explanation above, the existing hv_netvsc code is correct.
Thanks,
- Haiyang
^ permalink raw reply
* Is it safe for a NIC driver to use all the 48 bytes of skb->cb?
From: Dexuan Cui @ 2020-02-15 5:23 UTC (permalink / raw)
To: Haiyang Zhang, Stephen Hemminger, David S. Miller,
netdev@vger.kernel.org, KY Srinivasan,
linux-kernel@vger.kernel.org
Cc: linux-hyperv@vger.kernel.org
Hi,
It looks all the layers of drivers among the network stack can use the 48-byte
skb->cb array. Is there any rule how they should coordinate with each other?
I noticed the last 16 bytes are used by struct skb_gso_cb:
include/linux/skbuff.h:
struct skb_gso_cb {
union {
int mac_offset;
int data_offset;
};
int encap_level;
__wsum csum;
__u16 csum_start;
};
#define SKB_SGO_CB_OFFSET 32
#define SKB_GSO_CB(skb) ((struct skb_gso_cb *)((skb)->cb + SKB_SGO_CB_OFFSET))
Does this mean a low level NIC driver (e.g. hv_netvsc) should only use
the first 32 bytes? What if the upper layer network stack starts to take up
more space in the future?
Now hv_netvsc assumes it can use all of the 48-bytes, though it uses only
20 bytes, but just in case the struct hv_netvsc_packet grows to >32 bytes in the
future, should we change the BUILD_BUG_ON() in netvsc_start_xmit() to
BUILD_BUG_ON(sizeof(struct hv_netvsc_packet) > SKB_SGO_CB_OFFSET);
?
struct hv_netvsc_packet {
/* Bookkeeping stuff */
u8 cp_partial; /* partial copy into send buffer */
u8 rmsg_size; /* RNDIS header and PPI size */
u8 rmsg_pgcnt; /* page count of RNDIS header and PPI */
u8 page_buf_cnt;
u16 q_idx;
u16 total_packets;
u32 total_bytes;
u32 send_buf_index;
u32 total_data_buflen;
};
static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
{
...
/*
* Place the rndis header in the skb head room and
* the skb->cb will be used for hv_netvsc_packet
* structure.
*/
ret = skb_cow_head(skb, RNDIS_AND_PPI_SIZE);
if (ret)
goto no_memory;
/* Use the skb control buffer for building up the packet */
BUILD_BUG_ON(sizeof(struct hv_netvsc_packet) >
FIELD_SIZEOF(struct sk_buff, cb));
packet = (struct hv_netvsc_packet *)skb->cb;
Thanks,
-- Dexuan
^ permalink raw reply
* [PATCH AUTOSEL 5.5 389/542] clocksource/drivers/hyper-v: Reserve PAGE_SIZE space for tsc page
From: Sasha Levin @ 2020-02-14 15:46 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Boqun Feng, linux-hyperv, Michael Kelley, Daniel Lezcano,
Sasha Levin
In-Reply-To: <20200214154854.6746-1-sashal@kernel.org>
From: Boqun Feng <boqun.feng@gmail.com>
[ Upstream commit ddc61bbc45017726a2b450350d476b4dc5ae25ce ]
Currently, the reserved size for a tsc page is 4K, which is enough for
communicating with hypervisor. However, in the case where we want to
export the tsc page to userspace (e.g. for vDSO to read the
clocksource), the tsc page should be at least PAGE_SIZE, otherwise, when
PAGE_SIZE is larger than 4K, extra kernel data will be mapped into
userspace, which means leaking kernel information.
Therefore reserve PAGE_SIZE space for tsc_pg as a preparation for the
vDSO support of ARM64 in the future. Also, while at it, replace all
reference to tsc_pg with hv_get_tsc_page() since it should be the only
interface to access tsc page.
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
Cc: linux-hyperv@vger.kernel.org
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20191126021723.4710-1-boqun.feng@gmail.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/clocksource/hyperv_timer.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index 287d8d58c21ac..b6ea3a2093c56 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -307,17 +307,20 @@ EXPORT_SYMBOL_GPL(hv_stimer_global_cleanup);
struct clocksource *hyperv_cs;
EXPORT_SYMBOL_GPL(hyperv_cs);
-static struct ms_hyperv_tsc_page tsc_pg __aligned(PAGE_SIZE);
+static union {
+ struct ms_hyperv_tsc_page page;
+ u8 reserved[PAGE_SIZE];
+} tsc_pg __aligned(PAGE_SIZE);
struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
{
- return &tsc_pg;
+ return &tsc_pg.page;
}
EXPORT_SYMBOL_GPL(hv_get_tsc_page);
static u64 notrace read_hv_clock_tsc(struct clocksource *arg)
{
- u64 current_tick = hv_read_tsc_page(&tsc_pg);
+ u64 current_tick = hv_read_tsc_page(hv_get_tsc_page());
if (current_tick == U64_MAX)
hv_get_time_ref_count(current_tick);
@@ -372,7 +375,7 @@ static bool __init hv_init_tsc_clocksource(void)
return false;
hyperv_cs = &hyperv_cs_tsc;
- phys_addr = virt_to_phys(&tsc_pg);
+ phys_addr = virt_to_phys(hv_get_tsc_page());
/*
* The Hyper-V TLFS specifies to preserve the value of reserved
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.5 481/542] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.
From: Sasha Levin @ 2020-02-14 15:47 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Wei Hu, kbuild test robot, Bartlomiej Zolnierkiewicz, Sasha Levin,
dri-devel, linux-fbdev, linux-hyperv
In-Reply-To: <20200214154854.6746-1-sashal@kernel.org>
From: Wei Hu <weh@microsoft.com>
[ Upstream commit 3a6fb6c4255c3893ab61e2bd4e9ae01ca6bbcd94 ]
On Hyper-V, Generation 1 VMs can directly use VM's physical memory for
their framebuffers. This can improve the efficiency of framebuffer and
overall performence for VM. The physical memory assigned to framebuffer
must be contiguous. We use CMA allocator to get contiguouse physicial
memory when the framebuffer size is greater than 4MB. For size under
4MB, we use alloc_pages to achieve this.
To enable framebuffer memory allocation from CMA, supply a kernel
parameter to give enough space to CMA allocator at boot time. For
example:
cma=130m
This gives 130MB memory to CAM allocator that can be allocated to
framebuffer. If this fails, we fall back to the old way of using
mmio for framebuffer.
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Wei Hu <weh@microsoft.com>
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/hyperv_fb.c | 182 +++++++++++++++++++++++++-------
2 files changed, 144 insertions(+), 39 deletions(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index aa9541bf964b9..f65991a67af28 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2215,6 +2215,7 @@ config FB_HYPERV
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
select FB_DEFERRED_IO
+ select DMA_CMA if HAVE_DMA_CONTIGUOUS && CMA
help
This framebuffer driver supports Microsoft Hyper-V Synthetic Video.
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 4cd27e5172a16..8cf39d98b2bdf 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -31,6 +31,16 @@
* "set-vmvideo" command. For example
* set-vmvideo -vmname name -horizontalresolution:1920 \
* -verticalresolution:1200 -resolutiontype single
+ *
+ * Gen 1 VMs also support direct using VM's physical memory for framebuffer.
+ * It could improve the efficiency and performance for framebuffer and VM.
+ * This requires to allocate contiguous physical memory from Linux kernel's
+ * CMA memory allocator. To enable this, supply a kernel parameter to give
+ * enough memory space to CMA allocator for framebuffer. For example:
+ * cma=130m
+ * This gives 130MB memory to CMA allocator that can be allocated to
+ * framebuffer. For reference, 8K resolution (7680x4320) takes about
+ * 127MB memory.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -228,7 +238,6 @@ struct synthvid_msg {
} __packed;
-
/* FB driver definitions and structures */
#define HVFB_WIDTH 1152 /* default screen width */
#define HVFB_HEIGHT 864 /* default screen height */
@@ -258,12 +267,15 @@ struct hvfb_par {
/* If true, the VSC notifies the VSP on every framebuffer change */
bool synchronous_fb;
+ /* If true, need to copy from deferred IO mem to framebuffer mem */
+ bool need_docopy;
+
struct notifier_block hvfb_panic_nb;
/* Memory for deferred IO and frame buffer itself */
unsigned char *dio_vp;
unsigned char *mmio_vp;
- unsigned long mmio_pp;
+ phys_addr_t mmio_pp;
/* Dirty rectangle, protected by delayed_refresh_lock */
int x1, y1, x2, y2;
@@ -434,7 +446,7 @@ static void synthvid_deferred_io(struct fb_info *p,
maxy = max_t(int, maxy, y2);
/* Copy from dio space to mmio address */
- if (par->fb_ready)
+ if (par->fb_ready && par->need_docopy)
hvfb_docopy(par, start, PAGE_SIZE);
}
@@ -751,12 +763,12 @@ static void hvfb_update_work(struct work_struct *w)
return;
/* Copy the dirty rectangle to frame buffer memory */
- for (j = y1; j < y2; j++) {
- hvfb_docopy(par,
- j * info->fix.line_length +
- (x1 * screen_depth / 8),
- (x2 - x1) * screen_depth / 8);
- }
+ if (par->need_docopy)
+ for (j = y1; j < y2; j++)
+ hvfb_docopy(par,
+ j * info->fix.line_length +
+ (x1 * screen_depth / 8),
+ (x2 - x1) * screen_depth / 8);
/* Refresh */
if (par->fb_ready && par->update)
@@ -801,7 +813,8 @@ static int hvfb_on_panic(struct notifier_block *nb,
par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
par->synchronous_fb = true;
info = par->info;
- hvfb_docopy(par, 0, dio_fb_size);
+ if (par->need_docopy)
+ hvfb_docopy(par, 0, dio_fb_size);
synthvid_update(info, 0, 0, INT_MAX, INT_MAX);
return NOTIFY_DONE;
@@ -940,6 +953,62 @@ static void hvfb_get_option(struct fb_info *info)
return;
}
+/*
+ * Allocate enough contiguous physical memory.
+ * Return physical address if succeeded or -1 if failed.
+ */
+static phys_addr_t hvfb_get_phymem(struct hv_device *hdev,
+ unsigned int request_size)
+{
+ struct page *page = NULL;
+ dma_addr_t dma_handle;
+ void *vmem;
+ phys_addr_t paddr = 0;
+ unsigned int order = get_order(request_size);
+
+ if (request_size == 0)
+ return -1;
+
+ if (order < MAX_ORDER) {
+ /* Call alloc_pages if the size is less than 2^MAX_ORDER */
+ page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
+ if (!page)
+ return -1;
+
+ paddr = (page_to_pfn(page) << PAGE_SHIFT);
+ } else {
+ /* Allocate from CMA */
+ hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
+
+ vmem = dma_alloc_coherent(&hdev->device,
+ round_up(request_size, PAGE_SIZE),
+ &dma_handle,
+ GFP_KERNEL | __GFP_NOWARN);
+
+ if (!vmem)
+ return -1;
+
+ paddr = virt_to_phys(vmem);
+ }
+
+ return paddr;
+}
+
+/* Release contiguous physical memory */
+static void hvfb_release_phymem(struct hv_device *hdev,
+ phys_addr_t paddr, unsigned int size)
+{
+ unsigned int order = get_order(size);
+
+ if (order < MAX_ORDER)
+ __free_pages(pfn_to_page(paddr >> PAGE_SHIFT), order);
+ else
+ dma_free_coherent(&hdev->device,
+ round_up(size, PAGE_SIZE),
+ phys_to_virt(paddr),
+ paddr);
+}
+
/* Get framebuffer memory from Hyper-V video pci space */
static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
@@ -949,22 +1018,61 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
void __iomem *fb_virt;
int gen2vm = efi_enabled(EFI_BOOT);
resource_size_t pot_start, pot_end;
+ phys_addr_t paddr;
int ret;
- dio_fb_size =
- screen_width * screen_height * screen_depth / 8;
+ info->apertures = alloc_apertures(1);
+ if (!info->apertures)
+ return -ENOMEM;
- if (gen2vm) {
- pot_start = 0;
- pot_end = -1;
- } else {
+ if (!gen2vm) {
pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
- PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
+ PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
if (!pdev) {
pr_err("Unable to find PCI Hyper-V video\n");
+ kfree(info->apertures);
return -ENODEV;
}
+ info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
+ info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
+
+ /*
+ * For Gen 1 VM, we can directly use the contiguous memory
+ * from VM. If we succeed, deferred IO happens directly
+ * on this allocated framebuffer memory, avoiding extra
+ * memory copy.
+ */
+ paddr = hvfb_get_phymem(hdev, screen_fb_size);
+ if (paddr != (phys_addr_t) -1) {
+ par->mmio_pp = paddr;
+ par->mmio_vp = par->dio_vp = __va(paddr);
+
+ info->fix.smem_start = paddr;
+ info->fix.smem_len = screen_fb_size;
+ info->screen_base = par->mmio_vp;
+ info->screen_size = screen_fb_size;
+
+ par->need_docopy = false;
+ goto getmem_done;
+ }
+ pr_info("Unable to allocate enough contiguous physical memory on Gen 1 VM. Using MMIO instead.\n");
+ } else {
+ info->apertures->ranges[0].base = screen_info.lfb_base;
+ info->apertures->ranges[0].size = screen_info.lfb_size;
+ }
+
+ /*
+ * Cannot use the contiguous physical memory.
+ * Allocate mmio space for framebuffer.
+ */
+ dio_fb_size =
+ screen_width * screen_height * screen_depth / 8;
+
+ if (gen2vm) {
+ pot_start = 0;
+ pot_end = -1;
+ } else {
if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
pci_resource_len(pdev, 0) < screen_fb_size) {
pr_err("Resource not available or (0x%lx < 0x%lx)\n",
@@ -993,20 +1101,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
if (par->dio_vp == NULL)
goto err3;
- info->apertures = alloc_apertures(1);
- if (!info->apertures)
- goto err4;
-
- if (gen2vm) {
- info->apertures->ranges[0].base = screen_info.lfb_base;
- info->apertures->ranges[0].size = screen_info.lfb_size;
- remove_conflicting_framebuffers(info->apertures,
- KBUILD_MODNAME, false);
- } else {
- info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
- info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
- }
-
/* Physical address of FB device */
par->mmio_pp = par->mem->start;
/* Virtual address of FB device */
@@ -1017,13 +1111,15 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
info->screen_base = par->dio_vp;
info->screen_size = dio_fb_size;
+getmem_done:
+ remove_conflicting_framebuffers(info->apertures,
+ KBUILD_MODNAME, false);
if (!gen2vm)
pci_dev_put(pdev);
+ kfree(info->apertures);
return 0;
-err4:
- vfree(par->dio_vp);
err3:
iounmap(fb_virt);
err2:
@@ -1032,18 +1128,25 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
err1:
if (!gen2vm)
pci_dev_put(pdev);
+ kfree(info->apertures);
return -ENOMEM;
}
/* Release the framebuffer */
-static void hvfb_putmem(struct fb_info *info)
+static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
{
struct hvfb_par *par = info->par;
- vfree(par->dio_vp);
- iounmap(info->screen_base);
- vmbus_free_mmio(par->mem->start, screen_fb_size);
+ if (par->need_docopy) {
+ vfree(par->dio_vp);
+ iounmap(info->screen_base);
+ vmbus_free_mmio(par->mem->start, screen_fb_size);
+ } else {
+ hvfb_release_phymem(hdev, info->fix.smem_start,
+ screen_fb_size);
+ }
+
par->mem = NULL;
}
@@ -1062,6 +1165,7 @@ static int hvfb_probe(struct hv_device *hdev,
par = info->par;
par->info = info;
par->fb_ready = false;
+ par->need_docopy = true;
init_completion(&par->wait);
INIT_DELAYED_WORK(&par->dwork, hvfb_update_work);
@@ -1147,7 +1251,7 @@ static int hvfb_probe(struct hv_device *hdev,
error:
fb_deferred_io_cleanup(info);
- hvfb_putmem(info);
+ hvfb_putmem(hdev, info);
error2:
vmbus_close(hdev->channel);
error1:
@@ -1177,7 +1281,7 @@ static int hvfb_remove(struct hv_device *hdev)
vmbus_close(hdev->channel);
hv_set_drvdata(hdev, NULL);
- hvfb_putmem(info);
+ hvfb_putmem(hdev, info);
framebuffer_release(info);
return 0;
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.4 336/459] clocksource/drivers/hyper-v: Reserve PAGE_SIZE space for tsc page
From: Sasha Levin @ 2020-02-14 15:59 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Boqun Feng, linux-hyperv, Michael Kelley, Daniel Lezcano,
Sasha Levin
In-Reply-To: <20200214160149.11681-1-sashal@kernel.org>
From: Boqun Feng <boqun.feng@gmail.com>
[ Upstream commit ddc61bbc45017726a2b450350d476b4dc5ae25ce ]
Currently, the reserved size for a tsc page is 4K, which is enough for
communicating with hypervisor. However, in the case where we want to
export the tsc page to userspace (e.g. for vDSO to read the
clocksource), the tsc page should be at least PAGE_SIZE, otherwise, when
PAGE_SIZE is larger than 4K, extra kernel data will be mapped into
userspace, which means leaking kernel information.
Therefore reserve PAGE_SIZE space for tsc_pg as a preparation for the
vDSO support of ARM64 in the future. Also, while at it, replace all
reference to tsc_pg with hv_get_tsc_page() since it should be the only
interface to access tsc page.
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
Cc: linux-hyperv@vger.kernel.org
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20191126021723.4710-1-boqun.feng@gmail.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/clocksource/hyperv_timer.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index 2317d4e3daaff..bcac936fa62bd 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -213,17 +213,20 @@ EXPORT_SYMBOL_GPL(hv_stimer_global_cleanup);
struct clocksource *hyperv_cs;
EXPORT_SYMBOL_GPL(hyperv_cs);
-static struct ms_hyperv_tsc_page tsc_pg __aligned(PAGE_SIZE);
+static union {
+ struct ms_hyperv_tsc_page page;
+ u8 reserved[PAGE_SIZE];
+} tsc_pg __aligned(PAGE_SIZE);
struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
{
- return &tsc_pg;
+ return &tsc_pg.page;
}
EXPORT_SYMBOL_GPL(hv_get_tsc_page);
static u64 notrace read_hv_clock_tsc(struct clocksource *arg)
{
- u64 current_tick = hv_read_tsc_page(&tsc_pg);
+ u64 current_tick = hv_read_tsc_page(hv_get_tsc_page());
if (current_tick == U64_MAX)
hv_get_time_ref_count(current_tick);
@@ -278,7 +281,7 @@ static bool __init hv_init_tsc_clocksource(void)
return false;
hyperv_cs = &hyperv_cs_tsc;
- phys_addr = virt_to_phys(&tsc_pg);
+ phys_addr = virt_to_phys(hv_get_tsc_page());
/*
* The Hyper-V TLFS specifies to preserve the value of reserved
--
2.20.1
^ permalink raw reply related
* RE: [PATCH AUTOSEL 5.5 389/542] clocksource/drivers/hyper-v: Reserve PAGE_SIZE space for tsc page
From: Michael Kelley @ 2020-02-14 16:11 UTC (permalink / raw)
To: Sasha Levin, linux-kernel@vger.kernel.org, stable@vger.kernel.org
Cc: Boqun Feng, linux-hyperv@vger.kernel.org, Daniel Lezcano
In-Reply-To: <20200214154854.6746-389-sashal@kernel.org>
This patch does not need to be backported to any stable releases. It is prep work for guests on Hyper-V ARM64 when the guest page size is 16K or 64K, and that functionality isn't upstream yet.
Michael
> -----Original Message-----
> From: Sasha Levin <sashal@kernel.org>
> Sent: Friday, February 14, 2020 7:46 AM
> To: linux-kernel@vger.kernel.org; stable@vger.kernel.org
> Cc: Boqun Feng <boqun.feng@gmail.com>; linux-hyperv@vger.kernel.org; Michael Kelley
> <mikelley@microsoft.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; Sasha Levin
> <sashal@kernel.org>
> Subject: [PATCH AUTOSEL 5.5 389/542] clocksource/drivers/hyper-v: Reserve PAGE_SIZE
> space for tsc page
>
> From: Boqun Feng <boqun.feng@gmail.com>
>
> [ Upstream commit ddc61bbc45017726a2b450350d476b4dc5ae25ce ]
>
> Currently, the reserved size for a tsc page is 4K, which is enough for
> communicating with hypervisor. However, in the case where we want to
> export the tsc page to userspace (e.g. for vDSO to read the
> clocksource), the tsc page should be at least PAGE_SIZE, otherwise, when
> PAGE_SIZE is larger than 4K, extra kernel data will be mapped into
> userspace, which means leaking kernel information.
>
> Therefore reserve PAGE_SIZE space for tsc_pg as a preparation for the
> vDSO support of ARM64 in the future. Also, while at it, replace all
> reference to tsc_pg with hv_get_tsc_page() since it should be the only
> interface to access tsc page.
>
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> Cc: linux-hyperv@vger.kernel.org
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Link:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr
> %2F20191126021723.4710-1-
> boqun.feng%40gmail.com&data=02%7C01%7Cmikelley%40microsoft.com%7C2f481a9
> 3db624cb4b5e208d7b16691f6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6371
> 72926408924092&sdata=%2F1JNMBiq7l1ufSsWphZb%2FMLNep2EIOCzlXcG%2F9foZNw
> %3D&reserved=0
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/clocksource/hyperv_timer.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index 287d8d58c21ac..b6ea3a2093c56 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -307,17 +307,20 @@ EXPORT_SYMBOL_GPL(hv_stimer_global_cleanup);
> struct clocksource *hyperv_cs;
> EXPORT_SYMBOL_GPL(hyperv_cs);
>
> -static struct ms_hyperv_tsc_page tsc_pg __aligned(PAGE_SIZE);
> +static union {
> + struct ms_hyperv_tsc_page page;
> + u8 reserved[PAGE_SIZE];
> +} tsc_pg __aligned(PAGE_SIZE);
>
> struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
> {
> - return &tsc_pg;
> + return &tsc_pg.page;
> }
> EXPORT_SYMBOL_GPL(hv_get_tsc_page);
>
> static u64 notrace read_hv_clock_tsc(struct clocksource *arg)
> {
> - u64 current_tick = hv_read_tsc_page(&tsc_pg);
> + u64 current_tick = hv_read_tsc_page(hv_get_tsc_page());
>
> if (current_tick == U64_MAX)
> hv_get_time_ref_count(current_tick);
> @@ -372,7 +375,7 @@ static bool __init hv_init_tsc_clocksource(void)
> return false;
>
> hyperv_cs = &hyperv_cs_tsc;
> - phys_addr = virt_to_phys(&tsc_pg);
> + phys_addr = virt_to_phys(hv_get_tsc_page());
>
> /*
> * The Hyper-V TLFS specifies to preserve the value of reserved
> --
> 2.20.1
^ permalink raw reply
* RE: [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
From: Dexuan Cui @ 2020-02-13 8:05 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-pci@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Michael Kelley,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86@kernel.org, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
Andrew Murray
In-Reply-To: <20200213071407.GD69108@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
> From: Boqun Feng <boqun.feng@gmail.com>
> Sent: Wednesday, February 12, 2020 11:14 PM
>
> On Thu, Feb 13, 2020 at 04:18:01AM +0000, Dexuan Cui wrote:
> > > From: linux-hyperv-owner@vger.kernel.org
> > > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> > > b/arch/x86/include/asm/hyperv-tlfs.h
> > >
> > > +union hv_msi_entry {
> > > + u64 as_uint64;
> > > + struct {
> > > + u32 address;
> > > + u32 data;
> > > + } __packed;
> > > +};
> >
> > Just a small thing: should we move the __packed to after the "}" of
> > the union hv_msi_entry ?
> >
>
> Actually, in TLFS header, it's common to put the "__packed" inside the
> union, rather than after the union. It makes sense because union is
> different than struct: the alignment requirement of a union is already
> decided by the "as_*" member, so no need for "__packed" attribute.
>
> Regards,
> Boqun
I see. Thanks for the explanation!
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
From: Dexuan Cui @ 2020-02-13 8:04 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-pci@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Michael Kelley,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86@kernel.org, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
Andrew Murray
In-Reply-To: <20200213072623.GE69108@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
> From: Boqun Feng <boqun.feng@gmail.com>
> Sent: Wednesday, February 12, 2020 11:26 PM
>
> > Just a small thing: would it be slightly better if we change the name
> > in the above line to HVCALL_RETARGET_INTERRUPT ?
> >
> > HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the
> > actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is
> > used several times in the patchset so IMO we'd better always use
> > the same name.
>
> This might be a good suggestion, however, throughout the TLFS header,
> camel case is more commonly used for referencing hypercall. For example:
>
> /* HvCallSendSyntheticClusterIpi hypercall */
>
> So I think it's better to let it as it is for this patch, and later on,
> if we reach a consensus, we can convert the names all together.
>
> Thoughts?
>
> Regards,
> Boqun
Makes sense to me. Thanks for the explanation!
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
From: Boqun Feng @ 2020-02-13 7:26 UTC (permalink / raw)
To: Dexuan Cui
Cc: linux-pci@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Michael Kelley,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86@kernel.org, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
Andrew Murray
In-Reply-To: <HK0P153MB01481A125819FC7660E067AFBF1A0@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
On Thu, Feb 13, 2020 at 04:17:34AM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> > Sent: Sunday, February 9, 2020 7:40 PM
> >
> > Currently, retarget_msi_interrupt and other structures it relys on are
> > defined in pci-hyperv.c. However, those structures are actually defined
> > in Hypervisor Top-Level Functional Specification [1] and may be
> > different in sizes of fields or layout from architecture to
> > architecture. Let's move those definitions into x86's tlfs header file
> > to support virtual PCI on non-x86 architectures in the future. Note that
> > "__packed" attribute is added to these structures during the movement
> > for the same reason as we use the attribute for other TLFS structures in
> > the header file: make sure the structures meet the specification and
> > avoid anything unexpected from the compilers.
> >
> > Additionally, rename struct retarget_msi_interrupt to
> > hv_retarget_msi_interrupt for the consistent naming convention, also
> > mirroring the name in TLFS.
> >
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> > b/arch/x86/include/asm/hyperv-tlfs.h
> > +
> > +struct hv_device_interrupt_target {
> > + u32 vector;
> > + u32 flags;
> > + union {
> > + u64 vp_mask;
> > + struct hv_vpset vp_set;
> > + };
> > +} __packed;
> > +
> > +/* HvRetargetDeviceInterrupt hypercall */
>
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
>
Thanks!
> Just a small thing: would it be slightly better if we change the name
> in the above line to HVCALL_RETARGET_INTERRUPT ?
>
> HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the
> actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is
> used several times in the patchset so IMO we'd better always use
> the same name.
This might be a good suggestion, however, throughout the TLFS header,
camel case is more commonly used for referencing hypercall. For example:
/* HvCallSendSyntheticClusterIpi hypercall */
So I think it's better to let it as it is for this patch, and later on,
if we reach a consensus, we can convert the names all together.
Thoughts?
Regards,
Boqun
^ permalink raw reply
* Re: [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
From: Boqun Feng @ 2020-02-13 7:14 UTC (permalink / raw)
To: Dexuan Cui
Cc: linux-pci@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Michael Kelley,
KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86@kernel.org, Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
Andrew Murray
In-Reply-To: <HK0P153MB0148834D630E95D055CE051CBF1A0@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
On Thu, Feb 13, 2020 at 04:18:01AM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> > diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> > b/arch/x86/include/asm/hyperv-tlfs.h
> >
> > +union hv_msi_entry {
> > + u64 as_uint64;
> > + struct {
> > + u32 address;
> > + u32 data;
> > + } __packed;
> > +};
>
> Just a small thing: should we move the __packed to after the "}" of
> the union hv_msi_entry ?
>
Actually, in TLFS header, it's common to put the "__packed" inside the
union, rather than after the union. It makes sense because union is
different than struct: the alignment requirement of a union is already
decided by the "as_*" member, so no need for "__packed" attribute.
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
Thanks!
Regards,
Boqun
^ permalink raw reply
* RE: [PATCH v3 3/3] PCI: hv: Introduce hv_msi_entry
From: Dexuan Cui @ 2020-02-13 4:18 UTC (permalink / raw)
To: Boqun Feng, linux-pci@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, x86@kernel.org, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Andrew Murray
In-Reply-To: <20200210033953.99692-4-boqun.feng@gmail.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> b/arch/x86/include/asm/hyperv-tlfs.h
>
> +union hv_msi_entry {
> + u64 as_uint64;
> + struct {
> + u32 address;
> + u32 data;
> + } __packed;
> +};
Just a small thing: should we move the __packed to after the "}" of
the union hv_msi_entry ?
Reviewed-by: Dexuan Cui <decui@microsoft.com>
^ permalink raw reply
* RE: [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
From: Dexuan Cui @ 2020-02-13 4:17 UTC (permalink / raw)
To: Boqun Feng, linux-pci@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, x86@kernel.org, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Andrew Murray
In-Reply-To: <20200210033953.99692-3-boqun.feng@gmail.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> Sent: Sunday, February 9, 2020 7:40 PM
>
> Currently, retarget_msi_interrupt and other structures it relys on are
> defined in pci-hyperv.c. However, those structures are actually defined
> in Hypervisor Top-Level Functional Specification [1] and may be
> different in sizes of fields or layout from architecture to
> architecture. Let's move those definitions into x86's tlfs header file
> to support virtual PCI on non-x86 architectures in the future. Note that
> "__packed" attribute is added to these structures during the movement
> for the same reason as we use the attribute for other TLFS structures in
> the header file: make sure the structures meet the specification and
> avoid anything unexpected from the compilers.
>
> Additionally, rename struct retarget_msi_interrupt to
> hv_retarget_msi_interrupt for the consistent naming convention, also
> mirroring the name in TLFS.
>
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h
> b/arch/x86/include/asm/hyperv-tlfs.h
> +
> +struct hv_device_interrupt_target {
> + u32 vector;
> + u32 flags;
> + union {
> + u64 vp_mask;
> + struct hv_vpset vp_set;
> + };
> +} __packed;
> +
> +/* HvRetargetDeviceInterrupt hypercall */
Reviewed-by: Dexuan Cui <decui@microsoft.com>
Just a small thing: would it be slightly better if we change the name
in the above line to HVCALL_RETARGET_INTERRUPT ?
HVCALL_RETARGET_INTERRUPT is a define, so it may help to locate the
actual value of the define here. And, HVCALL_RETARGET_INTERRUPT is
used several times in the patchset so IMO we'd better always use
the same name.
^ permalink raw reply
* RE: [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header
From: Dexuan Cui @ 2020-02-13 4:17 UTC (permalink / raw)
To: Boqun Feng, linux-pci@vger.kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Cc: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, x86@kernel.org, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, Andrew Murray
In-Reply-To: <20200210033953.99692-2-boqun.feng@gmail.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Boqun Feng
> Sent: Sunday, February 9, 2020 7:40 PM
>
> Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are
> defined
> in pci-hyperv.c. However, similar to other hypercall related
> definitions, it makes more sense to put them in the tlfs header file.
>
> Besides, these definitions are arch-dependent, so for the support of
> virtual PCI on non-x86 archs in the future, move them into arch-specific
> tlfs header file.
>
> Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
Reviewed-by: Dexuan Cui <decui@microsoft.com>
^ permalink raw reply
* RE: [PATCH] PCI: hv: Replace zero-length array with flexible-array member
From: Dexuan Cui @ 2020-02-13 3:43 UTC (permalink / raw)
To: Gustavo A. R. Silva, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas
Cc: linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20200213005048.GA9662@embeddedor.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Gustavo A. R. Silva
> Sent: Wednesday, February 12, 2020 4:51 PM
> ...
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> This issue was found with the help of Coccinelle.
Looks good to me. Thanks, Gustavo!
Reviewed-by: Dexuan Cui <decui@microsoft.com>
FWIW, it looks there are a lot of more to fix in the kernel tree: the below
commands return 1373 for me:
grep -nr '\[0\];$' * | grep '\.h:' | grep -v = | wc -l
Running the commands against the kernel/ directory returns 3.
Thanks,
-- Dexuan
^ permalink raw reply
* [PATCH] PCI: hv: Replace zero-length array with flexible-array member
From: Gustavo A. R. Silva @ 2020-02-13 0:50 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas
Cc: linux-hyperv, linux-pci, linux-kernel, Gustavo A. R. Silva
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array member[1][2],
introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof operator
may not be applied. As a quirk of the original implementation of
zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/pci/controller/pci-hyperv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 9977abff92fc..be957268f9d6 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -260,7 +260,7 @@ struct pci_packet {
int resp_packet_size);
void *compl_ctxt;
- struct pci_message message[0];
+ struct pci_message message[];
};
/*
@@ -296,7 +296,7 @@ struct pci_bus_d0_entry {
struct pci_bus_relations {
struct pci_incoming_message incoming;
u32 device_count;
- struct pci_function_description func[0];
+ struct pci_function_description func[];
} __packed;
struct pci_q_res_req_response {
@@ -508,7 +508,7 @@ struct hv_dr_work {
struct hv_dr_state {
struct list_head list_entry;
u32 device_count;
- struct pci_function_description func[0];
+ struct pci_function_description func[];
};
enum hv_pcichild_state {
--
2.23.0
^ permalink raw reply related
* [PATCH v3 1/3] PCI: hv: Move hypercall related definitions into tlfs header
From: Boqun Feng @ 2020-02-10 3:39 UTC (permalink / raw)
To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
Andrew Murray, Bjorn Helgaas, Boqun Feng, Andrew Murray
In-Reply-To: <20200210033953.99692-1-boqun.feng@gmail.com>
Currently HVCALL_RETARGET_INTERRUPT and HV_PARTITION_ID_SELF are defined
in pci-hyperv.c. However, similar to other hypercall related
definitions, it makes more sense to put them in the tlfs header file.
Besides, these definitions are arch-dependent, so for the support of
virtual PCI on non-x86 archs in the future, move them into arch-specific
tlfs header file.
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
Reviewed-by: Andrew Murray <amurray@thegoodpenguin.co.uk>
---
arch/x86/include/asm/hyperv-tlfs.h | 3 +++
drivers/pci/controller/pci-hyperv.c | 6 ------
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 92abc1e42bfc..dffed0e10a68 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,8 @@ enum HV_GENERIC_SET_FORMAT {
HV_GENERIC_SET_ALL,
};
+#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 v3 3/3] PCI: hv: Introduce hv_msi_entry
From: Boqun Feng @ 2020-02-10 3:39 UTC (permalink / raw)
To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
Andrew Murray, Bjorn Helgaas, Boqun Feng, Andrew Murray
In-Reply-To: <20200210033953.99692-1-boqun.feng@gmail.com>
Add a new structure (hv_msi_entry), which is also defined in the TLFS,
to describe the msi entry for HVCALL_RETARGET_INTERRUPT. The structure
is needed because its layout may be different from architecture to
architecture.
Also add a new generic interface hv_set_msi_entry_from_desc() to allow
different archs to set the msi entry from msi_desc.
No functional change, only preparation for the future support of virtual
PCI on non-x86 architectures.
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
---
arch/x86/include/asm/hyperv-tlfs.h | 11 +++++++++--
arch/x86/include/asm/mshyperv.h | 8 ++++++++
drivers/pci/controller/pci-hyperv.c | 3 +--
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index a0b6a88d2f05..29336574d0bc 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -913,11 +913,18 @@ 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;
- u32 address;
- u32 data;
+ union hv_msi_entry msi_entry;
} __packed;
/*
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index 6b79515abb82..81fc30240122 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -4,6 +4,7 @@
#include <linux/types.h>
#include <linux/nmi.h>
+#include <linux/msi.h>
#include <asm/io.h>
#include <asm/hyperv-tlfs.h>
#include <asm/nospec-branch.h>
@@ -240,6 +241,13 @@ bool hv_vcpu_is_preempted(int vcpu);
static inline void hv_apic_init(void) {}
#endif
+static inline void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
+ struct msi_desc *msi_desc)
+{
+ msi_entry->address = msi_desc->msg.address_lo;
+ msi_entry->data = msi_desc->msg.data;
+}
+
#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 0d9b74503577..3f9b220c23ec 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1170,8 +1170,7 @@ 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_entry_from_desc(¶ms->int_entry.msi_entry, msi_desc);
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
* [PATCH v3 2/3] PCI: hv: Move retarget related structures into tlfs header
From: Boqun Feng @ 2020-02-10 3:39 UTC (permalink / raw)
To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
Andrew Murray, Bjorn Helgaas, Boqun Feng, Andrew Murray
In-Reply-To: <20200210033953.99692-1-boqun.feng@gmail.com>
Currently, retarget_msi_interrupt and other structures it relys on are
defined in pci-hyperv.c. However, those structures are actually defined
in Hypervisor Top-Level Functional Specification [1] and may be
different in sizes of fields or layout from architecture to
architecture. Let's move those definitions into x86's tlfs header file
to support virtual PCI on non-x86 architectures in the future. Note that
"__packed" attribute is added to these structures during the movement
for the same reason as we use the attribute for other TLFS structures in
the header file: make sure the structures meet the specification and
avoid anything unexpected from the compilers.
Additionally, rename struct retarget_msi_interrupt to
hv_retarget_msi_interrupt for the consistent naming convention, also
mirroring the name in TLFS.
[1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
---
arch/x86/include/asm/hyperv-tlfs.h | 31 ++++++++++++++++++++++++++
drivers/pci/controller/pci-hyperv.c | 34 ++---------------------------
2 files changed, 33 insertions(+), 32 deletions(-)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index dffed0e10a68..a0b6a88d2f05 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -912,4 +912,35 @@ struct hv_tlb_flush_ex {
struct hv_partition_assist_pg {
u32 tlb_lock_count;
};
+
+struct hv_interrupt_entry {
+ u32 source; /* 1 for MSI(-X) */
+ u32 reserved1;
+ u32 address;
+ u32 data;
+} __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; /* use "self" */
+ 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/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index aacfcc90d929..0d9b74503577 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;
--
2.24.1
^ permalink raw reply related
* [PATCH v3 0/3] PCI: hv: Generify pci-hyperv.c
From: Boqun Feng @ 2020-02-10 3:39 UTC (permalink / raw)
To: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel
Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
Andrew Murray, Bjorn Helgaas, Boqun Feng
Hi,
This is the first part for virtual PCI support of Hyper-V guest on
ARM64. The whole patchset doesn't have any functional change, but only
refactors the pci-hyperv.c code to make it more arch-independent.
Previous version:
v1: https://lore.kernel.org/lkml/20200121015713.69691-1-boqun.feng@gmail.com/
v2: https://lore.kernel.org/linux-arm-kernel/20200203050313.69247-1-boqun.feng@gmail.com/
Changes since v2:
* Rebased on 5.6-rc1
* Reword commit logs as per Andrew's suggestion.
* It makes more sense to have a generic interface to set the whole
msi_entry rather than only the "address" field. So change
hv_set_msi_address_from_desc() to hv_set_msi_entry_from_desc().
Additionally, make it an inline function as per the suggestion
of Andrew and Thomas.
* Add the missing comment saying the partition_id of
hv_retarget_device_interrupt must be self.
* Add the explanation for why "__packed" is needed for TLFS
structures.
I've done compile and boot test of this patchset, also done some tests
with a pass-through NVMe device.
Suggestions and comments are welcome!
Regards,
Boqun
Boqun Feng (3):
PCI: hv: Move hypercall related definitions into tlfs header
PCI: hv: Move retarget related structures into tlfs header
PCI: hv: Introduce hv_msi_entry
arch/x86/include/asm/hyperv-tlfs.h | 41 +++++++++++++++++++++++++++
arch/x86/include/asm/mshyperv.h | 8 ++++++
drivers/pci/controller/pci-hyperv.c | 43 ++---------------------------
3 files changed, 52 insertions(+), 40 deletions(-)
--
2.24.1
^ permalink raw reply
* Re: [PATCH net] hv_netvsc: Fix XDP refcnt for synthetic and VF NICs
From: David Miller @ 2020-02-07 10:34 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <1581026465-36161-1-git-send-email-haiyangz@microsoft.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Thu, 6 Feb 2020 14:01:05 -0800
> The caller of XDP_SETUP_PROG has already incremented refcnt in
> __bpf_prog_get(), so drivers should only increment refcnt by
> num_queues - 1.
>
> To fix the issue, update netvsc_xdp_set() to add the correct number
> to refcnt.
>
> Hold a refcnt in netvsc_xdp_set()’s other caller, netvsc_attach().
>
> And, do the same in netvsc_vf_setxdp(). Otherwise, every time when VF is
> removed and added from the host side, the refcnt will be decreased by one,
> which may cause page fault when unloading xdp program.
>
> Fixes: 351e1581395f ("hv_netvsc: Add XDP support")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH v2 2/3] PCI: hv: Move retarget related structures into tlfs header
From: Boqun Feng @ 2020-02-07 7:58 UTC (permalink / raw)
To: Andrew Murray
Cc: linux-pci, linux-hyperv, linux-kernel, linux-arm-kernel,
Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, x86, Lorenzo Pieralisi,
Andrew Murray, Bjorn Helgaas
In-Reply-To: <20200203140902.GF83200@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
On Mon, Feb 03, 2020 at 10:09:02PM +0800, Boqun Feng wrote:
[...]
> > > mirroring the name in TLFS.
> > >
> > > [1]: https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs
> > >
> > > Signed-off-by: Boqun Feng (Microsoft) <boqun.feng@gmail.com>
> > > ---
> > > arch/x86/include/asm/hyperv-tlfs.h | 31 ++++++++++++++++++++++++++
> > > drivers/pci/controller/pci-hyperv.c | 34 ++---------------------------
> > > 2 files changed, 33 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> > > index 739bd89226a5..4a76e442481a 100644
> > > --- a/arch/x86/include/asm/hyperv-tlfs.h
> > > +++ b/arch/x86/include/asm/hyperv-tlfs.h
> > > @@ -911,4 +911,35 @@ struct hv_tlb_flush_ex {
> > > struct hv_partition_assist_pg {
> > > u32 tlb_lock_count;
> > > };
> > > +
> > > +struct hv_interrupt_entry {
> > > + u32 source; /* 1 for MSI(-X) */
> > > + u32 reserved1;
> > > + u32 address;
> > > + u32 data;
> > > +} __packed;
> >
> > Why have you added __packed here? There is no mention of this change in the
> > commit log? Is it needed?
> >
>
> I'm simply following the convention of hyperv-tlfs.h: most of the
> structures have this "__packed" attribute. I personally don't think this
> attribute is necessary, but I was afraid that I was missing something
> subtle. So a question for folks working on Hyper-V: why we need this
> attribute on TLFS-defined structures? Most of those will have no
> difference with or without this attribute, IIUC.
>
I find this patch:
https://lore.kernel.org/lkml/20181212175701.18754-1-vkuznets@redhat.com/
The reason why the "__packed" attribute is needed is to protect the
hypervisor-guet communication structures from unexpected behaviors of
compilers.
I will keep the code as it is and add some words in the commit log.
Regards,
Boqun
> > > +
> > > +/*
> > > + * 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;
> >
> > Same here.
> >
> > > +
> > > +/* HvRetargetDeviceInterrupt hypercall */
> > > +struct hv_retarget_device_interrupt {
> > > + u64 partition_id;
> >
> > Why drop the 'self' comment?
> >
>
> Good catch, TLFS does say this field must be 'self'. I will add it in
> next version.
>
> > > + 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/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > > index aacfcc90d929..0d9b74503577 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;
> >
> > pci-hyperv.c also makes use of retarget_msi_interrupt_lock - it's really clear
> > from this name what it protects, however your rename now makes this more
> > confusing.
> >
> > Likewise there is a comment in hv_pci_probe that refers to
> > retarget_msi_interrupt_params which is now stale.
> >
>
> But 'retarget_msi_interrupt_params' is the name of field in
> hv_pcibus_device, so is 'retarget_msi_interrupt_lock'. And what I change
> is the name of type. I believe people can tell the relationship from
> the name of the fields, and the comment of hv_pci_probe actually refers
> to the field rather than the type.
>
> > It may be helpful to rename hv_retarget_device_interrupt for consistency with
> > the docs - however please make sure you catch all the references - I'd suggest
> > that the move and the rename are in different patches.
> >
>
> If the renaming requires a lot of work (e.g. need to change multiple
> references), I will follow your suggestion. But seems it's not the case
> for this renaming.
>
> Regards,
> Boqun
>
> > Thanks,
> >
> > Andrew Murray
> >
> > > struct hv_pcibus_device *hbus;
> > > struct cpumask *dest;
> > > cpumask_var_t tmp;
> > > --
> > > 2.24.1
> > >
^ permalink raw reply
* RE: [Patch v4 1/2] PCI: hv: Decouple the func definition in hv_dr_state from VSP message
From: Long Li @ 2020-02-06 23:48 UTC (permalink / raw)
To: Long Li, longli@linuxonhyperv.com, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, linux-hyperv@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <SN6PR2101MB10234B91D7748441CA18A361CE0C0@SN6PR2101MB1023.namprd21.prod.outlook.com>
Hi guys,
Ping again...
Please take a look at these two patches.
Thanks
Long
>Subject: RE: [Patch v4 1/2] PCI: hv: Decouple the func definition in hv_dr_state
>from VSP message
>
>Hi Lorenzo,
>
>Can you take a look at this patch?
>
>Thanks
>
>Long
>
>>Subject: [Patch v4 1/2] PCI: hv: Decouple the func definition in
>>hv_dr_state from VSP message
>>
>>From: Long Li <longli@microsoft.com>
>>
>>hv_dr_state is used to find present PCI devices on the bus. The
>>structure reuses struct pci_function_description from VSP message to describe
>a device.
>>
>>To prepare support for pci_function_description v2, decouple this
>>dependence in hv_dr_state so it can work with both v1 and v2 VSP messages.
>>
>>There is no functionality change.
>>
>>Signed-off-by: Long Li <longli@microsoft.com>
>>Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>>---
>>Changes
>>v2: Changed some spaces to tabs, changed failure code to -ENOMEM
>>v3: Revised comment for function hv_pci_devices_present(), reformatted
>>patch title
>>v4: Fixed spelling
>>
>> drivers/pci/controller/pci-hyperv.c | 101 +++++++++++++++++++---------
>> 1 file changed, 70 insertions(+), 31 deletions(-)
>>
>>diff --git a/drivers/pci/controller/pci-hyperv.c
>>b/drivers/pci/controller/pci- hyperv.c index f1f300218fab..3b3e1967cf08
>>100644
>>--- a/drivers/pci/controller/pci-hyperv.c
>>+++ b/drivers/pci/controller/pci-hyperv.c
>>@@ -507,10 +507,24 @@ struct hv_dr_work {
>> struct hv_pcibus_device *bus;
>> };
>>
>>+struct hv_pcidev_description {
>>+ u16 v_id; /* vendor ID */
>>+ u16 d_id; /* device ID */
>>+ u8 rev;
>>+ u8 prog_intf;
>>+ u8 subclass;
>>+ u8 base_class;
>>+ u32 subsystem_id;
>>+ union win_slot_encoding win_slot;
>>+ u32 ser; /* serial number */
>>+ u32 flags;
>>+ u16 virtual_numa_node;
>>+};
>>+
>> struct hv_dr_state {
>> struct list_head list_entry;
>> u32 device_count;
>>- struct pci_function_description func[0];
>>+ struct hv_pcidev_description func[0];
>> };
>>
>> enum hv_pcichild_state {
>>@@ -527,7 +541,7 @@ struct hv_pci_dev {
>> refcount_t refs;
>> enum hv_pcichild_state state;
>> struct pci_slot *pci_slot;
>>- struct pci_function_description desc;
>>+ struct hv_pcidev_description desc;
>> bool reported_missing;
>> struct hv_pcibus_device *hbus;
>> struct work_struct wrk;
>>@@ -1862,7 +1876,7 @@ static void q_resource_requirements(void
>>*context, struct pci_response *resp,
>> * Return: Pointer to the new tracking struct
>> */
>> static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
>>- struct pci_function_description *desc)
>>+ struct hv_pcidev_description *desc)
>> {
>> struct hv_pci_dev *hpdev;
>> struct pci_child_message *res_req;
>>@@ -1973,7 +1987,7 @@ static void pci_devices_present_work(struct
>>work_struct *work) {
>> u32 child_no;
>> bool found;
>>- struct pci_function_description *new_desc;
>>+ struct hv_pcidev_description *new_desc;
>> struct hv_pci_dev *hpdev;
>> struct hv_pcibus_device *hbus;
>> struct list_head removed;
>>@@ -2090,43 +2104,26 @@ static void pci_devices_present_work(struct
>>work_struct *work)
>> put_hvpcibus(hbus);
>> kfree(dr);
>> }
>>-
>> /**
>>- * hv_pci_devices_present() - Handles list of new children
>>+ * hv_pci_start_relations_work() - Queue work to start device
>>+ discovery
>> * @hbus: Root PCI bus, as understood by this driver
>>- * @relations: Packet from host listing children
>>+ * @dr: The list of children returned from host
>> *
>>- * This function is invoked whenever a new list of devices for
>>- * this bus appears.
>>+ * Return: 0 on success, -errno on failure
>> */
>>-static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
>>- struct pci_bus_relations *relations)
>>+static int hv_pci_start_relations_work(struct hv_pcibus_device *hbus,
>>+ struct hv_dr_state *dr)
>> {
>>- struct hv_dr_state *dr;
>> struct hv_dr_work *dr_wrk;
>>- unsigned long flags;
>> bool pending_dr;
>>+ unsigned long flags;
>>
>> dr_wrk = kzalloc(sizeof(*dr_wrk), GFP_NOWAIT);
>> if (!dr_wrk)
>>- return;
>>-
>>- dr = kzalloc(offsetof(struct hv_dr_state, func) +
>>- (sizeof(struct pci_function_description) *
>>- (relations->device_count)), GFP_NOWAIT);
>>- if (!dr) {
>>- kfree(dr_wrk);
>>- return;
>>- }
>>+ return -ENOMEM;
>>
>> INIT_WORK(&dr_wrk->wrk, pci_devices_present_work);
>> dr_wrk->bus = hbus;
>>- dr->device_count = relations->device_count;
>>- if (dr->device_count != 0) {
>>- memcpy(dr->func, relations->func,
>>- sizeof(struct pci_function_description) *
>>- dr->device_count);
>>- }
>>
>> spin_lock_irqsave(&hbus->device_list_lock, flags);
>> /*
>>@@ -2144,6 +2141,47 @@ static void hv_pci_devices_present(struct
>>hv_pcibus_device *hbus,
>> get_hvpcibus(hbus);
>> queue_work(hbus->wq, &dr_wrk->wrk);
>> }
>>+
>>+ return 0;
>>+}
>>+
>>+/**
>>+ * hv_pci_devices_present() - Handle list of new children
>>+ * @hbus: Root PCI bus, as understood by this driver
>>+ * @relations: Packet from host listing children
>>+ *
>>+ * Process a new list of devices on the bus. The list of devices is
>>+ * discovered by VSP and sent to us via VSP message PCI_BUS_RELATIONS,
>>+ * whenever a new list of devices for this bus appears.
>>+ */
>>+static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
>>+ struct pci_bus_relations *relations) {
>>+ struct hv_dr_state *dr;
>>+ int i;
>>+
>>+ dr = kzalloc(offsetof(struct hv_dr_state, func) +
>>+ (sizeof(struct hv_pcidev_description) *
>>+ (relations->device_count)), GFP_NOWAIT);
>>+
>>+ if (!dr)
>>+ return;
>>+
>>+ dr->device_count = relations->device_count;
>>+ for (i = 0; i < dr->device_count; i++) {
>>+ dr->func[i].v_id = relations->func[i].v_id;
>>+ dr->func[i].d_id = relations->func[i].d_id;
>>+ dr->func[i].rev = relations->func[i].rev;
>>+ dr->func[i].prog_intf = relations->func[i].prog_intf;
>>+ dr->func[i].subclass = relations->func[i].subclass;
>>+ dr->func[i].base_class = relations->func[i].base_class;
>>+ dr->func[i].subsystem_id = relations->func[i].subsystem_id;
>>+ dr->func[i].win_slot = relations->func[i].win_slot;
>>+ dr->func[i].ser = relations->func[i].ser;
>>+ }
>>+
>>+ if (hv_pci_start_relations_work(hbus, dr))
>>+ kfree(dr);
>> }
>>
>> /**
>>@@ -3018,7 +3056,7 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
>> struct pci_packet teardown_packet;
>> u8 buffer[sizeof(struct pci_message)];
>> } pkt;
>>- struct pci_bus_relations relations;
>>+ struct hv_dr_state *dr;
>> struct hv_pci_compl comp_pkt;
>> int ret;
>>
>>@@ -3030,8 +3068,9 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
>> return;
>>
>> /* Delete any children which might still exist. */
>>- memset(&relations, 0, sizeof(relations));
>>- hv_pci_devices_present(hbus, &relations);
>>+ dr = kzalloc(sizeof(*dr), GFP_KERNEL);
>>+ if (dr && hv_pci_start_relations_work(hbus, dr))
>>+ kfree(dr);
>>
>> ret = hv_send_resources_released(hdev);
>> if (ret)
>>--
>>2.17.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox