Re: [Xen-devel] [PATCH 1/3 v3] xen: Refactor 16550 UART code
On Fri, Nov 24, 2017 at 05:09:10PM +0530, Bhupinder Thakur wrote:
> This patch refactors the 8250 UART code so that code can be reused
> by later patches, which add support for ACPI based UART
> initialization.
>
> Signed-off-by: Bhupinder Thakur
> ---
> Changes since v2:
> - Refactored the code to prepare for later patches.
>
> CC: Andrew Cooper
> CC: George Dunlap
> CC: Ian Jackson
> CC: Jan Beulich
> CC: Konrad Rzeszutek Wilk
> CC: Stefano Stabellini
> CC: Tim Deegan
> CC: Wei Liu
> CC: Julien Grall
>
> xen/drivers/char/ns16550.c | 53
> ++
> 1 file changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index e0f8199..c5dfc1e 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1462,16 +1462,32 @@ void __init ns16550_init(int index, struct
> ns16550_defaults *defaults)
> ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
> }
>
> +#ifdef CONFIG_ARM
> +static void ns16550_vuart_init(struct ns16550 *uart)
> +{
> +uart->vuart.base_addr = uart->io_base;
> +uart->vuart.size= uart->io_size;
> +uart->vuart.data_off= UART_THR << uart->reg_shift;
> +uart->vuart.status_off = UART_LSR << uart->reg_shift;
> +uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;
> +}
> +#endif
> +
> +static void ns16550_register_uart(struct ns16550 *uart)
> +{
> +/* Register with generic serial driver. */
> +serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +}
> +
> #ifdef CONFIG_HAS_DEVICE_TREE
> -static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> - const void *data)
> +
> +static int ns16550_init_dt(struct ns16550 **puart,
> + const struct dt_device_node *dev)
> {
> -struct ns16550 *uart;
> int res;
> u32 reg_shift, reg_width;
> u64 io_size;
> -
> -uart = &ns16550_com[0];
> +struct ns16550 *uart = &ns16550_com[0];
I am sure I understand this change. In the #2 patch you are doing
this the old way:
+static int ns16550_init_acpi(struct ns16550 **puart)
+{
+ struct acpi_table_spcr *spcr;
+int status;
+struct ns16550 *uart = &ns16550_com[0];
Any particular resaon for this change?
If you can restore this to the original way in this function you
can put:
Reviewed-by: Konrad Rzeszutek Wilk
>
> ns16550_init_common(uart);
>
> @@ -1510,18 +1526,29 @@ static int __init ns16550_uart_dt_init(struct
> dt_device_node *dev,
>
> uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
>
> -uart->vuart.base_addr = uart->io_base;
> -uart->vuart.size = uart->io_size;
> -uart->vuart.data_off = UART_THR <reg_shift;
> -uart->vuart.status_off = UART_LSR<reg_shift;
> -uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;
> +*puart = uart;
>
> -/* Register with generic serial driver. */
> -serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
> +return 0;
> +}
> +
> +static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> + const void *data)
> +{
> +struct ns16550 *uart;
> +int ret;
> +
> +ret = ns16550_init_dt(&uart, data);
> +
> +if ( ret )
> +return ret;
> +
> +ns16550_vuart_init(uart);
> +
> +ns16550_register_uart(uart);
>
> dt_device_set_used_by(dev, DOMID_XEN);
>
> -return 0;
> +return ret;
> }
>
> static const struct dt_device_match ns16550_dt_match[] __initconst =
> --
> 2.7.4
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI
On Fri, Nov 24, 2017 at 05:09:11PM +0530, Bhupinder Thakur wrote:
> Currently, Xen supports only DT based initialization of 16550 UART.
> This patch adds support for initializing 16550 UART using ACPI SPCR table.
Can you provide the link to the spec you are using. I am wondering
if I am looking at some older one.
>
> Signed-off-by: Bhupinder Thakur
> ---
> Changes since v2:
> - renamed UART_MAX_REG to UART_NUM_REGS
> - aligned some assignment statements
> - some coding style changes
>
> CC: Andrew Cooper
> CC: George Dunlap
> CC: Ian Jackson
> CC: Jan Beulich
> CC: Konrad Rzeszutek Wilk
> CC: Stefano Stabellini
> CC: Tim Deegan
> CC: Wei Liu
> CC: Julien Grall
>
> xen/drivers/char/ns16550.c | 67
> +
> xen/include/xen/8250-uart.h | 1 +
> 2 files changed, 68 insertions(+)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index c5dfc1e..af4712f 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -29,6 +29,10 @@
> #ifdef CONFIG_X86
> #include
> #endif
> +#ifdef CONFIG_ACPI
> +#include
> +#endif
> +
>
> /*
> * Configure serial port with a string:
> @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
> DT_DEVICE_END
>
> #endif /* HAS_DEVICE_TREE */
> +
> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
I would remove the CONFIG_ARM as the spec says it is possible to use
this on ARM _and_ x86 hardware.
But I guess you can't as ACPI_DEVICE_START is defined to be only
on ARM?
> +
> +static int ns16550_init_acpi(struct ns16550 **puart)
> +{
> +struct acpi_table_spcr *spcr;
> +int status;
hmm, not acpi_status ?
> +struct ns16550 *uart = &ns16550_com[0];
> +
> +ns16550_init_common(uart);
I would move this below the error check below..
> +
> +status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +(struct acpi_table_header **)&spcr);
> +
> +if ( ACPI_FAILURE(status) )
> +{
> +printk("ns16550: Failed to get SPCR table\n");
> +return -EINVAL;
> +}
> +
> +uart->baud = BAUD_AUTO;
> +uart->data_bits = 8;
Are those two assumed from the ACPI spec?
Wait a minute. The
https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
has Baud Rate at Offset 58?
> +uart->parity= spcr->parity;
> +uart->stop_bits = spcr->stop_bits;
> +uart->io_base = spcr->serial_port.address;
> +uart->irq = spcr->interrupt;
You need to check if the 'Interrupt Type' field is set before
you look at this?
> +uart->reg_width = spcr->serial_port.bit_width / 8;
> +uart->reg_shift = 0;
> +uart->io_size = UART_NUM_REGS << uart->reg_shift;
> +
> +irq_set_type(spcr->interrupt, spcr->interrupt_type);
Um, the spec has a whole bunch of other bits set in 'interrupt_type'?
> +
> +*puart = uart;
> +
> +return 0;
> +}
> +
> +static int __init ns16550_acpi_uart_init(const void *data)
> +{
> +int ret;
> +struct ns16550 *uart;
> +
> +ret = ns16550_init_acpi(&uart);
> +if ( ret )
> +return ret;
> +
> +ns16550_vuart_init(uart);
> +
> +ns16550_register_uart(uart);
> +
> +return 0;
> +}
> +
> +ACPI_DEVICE_START(ns16550c, "16550 COMPAT UART", DEVICE_SERIAL)
> +.class_type = ACPI_DBG2_16550_COMPATIBLE,
> +.init = ns16550_acpi_uart_init,
> +ACPI_DEVICE_END
> +ACPI_DEVICE_START(ns16550s, "16550 SUBSET UART", DEVICE_SERIAL)
> +.class_type = ACPI_DBG2_16550_SUBSET,
> +.init = ns16550_acpi_uart_init,
> +ACPI_DEVICE_END
> +
> +#endif
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index 5c3bac3..849a5c0 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -35,6 +35,7 @@
> #define UART_USR 0x1f/* Status register (DW) */
> #define UART_DLL 0x00/* divisor latch (ls) (DLAB=1) */
> #define UART_DLM 0x01/* divisor latch (ms) (DLAB=1) */
> +#define UART_NUM_REGS (UART_USR + 1)
>
> /* Interrupt Enable Register */
> #define UART_IER_ERDAI0x01/* rx data recv'd */
> --
> 2.7.4
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/3 v3] xen: Fix 16550 UART console for HP Moonshot (Aarch64) platform
On Fri, Nov 24, 2017 at 05:09:12PM +0530, Bhupinder Thakur wrote:
> The console was not working on HP Moonshot (HPE Proliant Aarch64) because
> the UART registers were accessed as 8-bit aligned addresses. However,
> registers are 32-bit aligned for HP Moonshot.
>
> Since ACPI/SPCR table does not specify the register shift to be applied to the
> register offset, this patch implements an erratum to correctly set the
> register
> shift for HP Moonshot.
>
> Similar erratum was implemented in linux in the following commit:
>
> commit 79a648328d2a604524a30523ca763fbeca0f70e3
> Author: Loc Ho
> Date: Mon Jul 3 14:33:09 2017 -0700
>
> ACPI: SPCR: Workaround for APM X-Gene 8250 UART 32-alignment errata
>
> APM X-Gene verion 1 and 2 have an 8250 UART with its register
> aligned to 32-bit. In addition, the latest released BIOS
> encodes the access field as 8-bit access instead 32-bit access.
> This causes no console with ACPI boot as the console
> will not match X-Gene UART port due to the lack of mmio32
> option.
>
> Signed-off-by: Loc Ho
> Acked-by: Greg Kroah-Hartman
> Signed-off-by: Rafael J. Wysocki
>
> Signed-off-by: Bhupinder Thakur
Reviewed-by: Konrad Rzeszutek Wilk
> ---
> Changes since v2:
> - removed the use of local variable xgene_8250 in xgene_8250_erratum_present
>
> CC: Andrew Cooper
> CC: George Dunlap
> CC: Ian Jackson
> CC: Jan Beulich
> CC: Konrad Rzeszutek Wilk
> CC: Stefano Stabellini
> CC: Tim Deegan
> CC: Wei Liu
> CC: Julien Grall
>
> xen/drivers/char/ns16550.c | 38 +-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index af4712f..8c4720a 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1571,6 +1571,30 @@ DT_DEVICE_END
> #endif /* HAS_DEVICE_TREE */
>
> #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
> +/*
> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
> + * register aligned to 32-bit. In addition, the BIOS also encoded the
> + * access width to be 8 bits. This function detects this errata condition.
> + */
> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
> +{
> +if ( tb->interface_type != ACPI_DBG2_16550_COMPATIBLE )
> +return false;
> +
> +if ( memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
> + memcmp(tb->header.oem_id, "HPE ", ACPI_OEM_ID_SIZE) )
> +return false;
> +
> +if ( !memcmp(tb->header.oem_table_id, "XGENESPC",
> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0 )
> +return true;
> +
> +if ( !memcmp(tb->header.oem_table_id, "ProLiant",
> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1 )
> +return true;
> +
> +return false;
> +}
>
> static int ns16550_init_acpi(struct ns16550 **puart)
> {
> @@ -1596,7 +1620,19 @@ static int ns16550_init_acpi(struct ns16550 **puart)
> uart->io_base = spcr->serial_port.address;
> uart->irq = spcr->interrupt;
> uart->reg_width = spcr->serial_port.bit_width / 8;
> -uart->reg_shift = 0;
> +
> +if ( xgene_8250_erratum_present(spcr) )
> +{
> +/*
> + * For xgene v1 and v2 the registers are 32-bit and so a
> + * register shift of 2 has to be applied to get the
> + * correct register offset.
> + */
> +uart->reg_shift = 2;
> +}
> +else
> +uart->reg_shift = 0;
> +
> uart->io_size = UART_NUM_REGS << uart->reg_shift;
>
> irq_set_type(spcr->interrupt, spcr->interrupt_type);
> --
> 2.7.4
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/setup: do not relocate below the end of current Xen image placement
On Mon, Nov 27, 2017 at 04:58:52PM +, Andrew Cooper wrote: > On 27/11/17 15:41, Daniel Kiper wrote: > > If it is possible we would like to have the Xen image higher than the > > booloader put it and certainly do not overwrite the Xen code and data > > during copy/relocation. Otherwise the Xen may crash silently at boot. > > > > Signed-off-by: Daniel Kiper > > Actually, there is a second related bug which I've only just got to the > bottom of, and haven't had time to fix yet. > > (XEN) Bootloader: GRUB 2.02 > (XEN) Command line: hvm_fep com1=115200,8n1 console=com1,vga > dom0_mem=2048M,max:2048M watchdog ucode=scan dom0_max_vcpus=8 > crashkernel=192M,below=4G > (XEN) Xen image load base address: 0xaba0 > (XEN) Video information: > (XEN) VGA is text mode 80x25, font 8x16 > (XEN) VBE/DDC methods: none; EDID transfer time: 0 seconds > (XEN) EDID info not retrieved because no DDC retrieval method detected > (XEN) Disc information: > (XEN) Found 1 MBR signatures > (XEN) Found 1 EDD information structures > (XEN) Xen-e820 RAM map: > (XEN) - 0009bc00 (usable) > (XEN) 0009bc00 - 000a (reserved) > (XEN) 000e - 0010 (reserved) > (XEN) 0010 - ac209000 (usable) > (XEN) ac209000 - aeb99000 (reserved) > (XEN) aeb99000 - aeb9d000 (ACPI NVS) > (XEN) aeb9d000 - aecd1000 (reserved) > (XEN) aecd1000 - aecd4000 (ACPI NVS) > (XEN) aecd4000 - aecf5000 (reserved) > (XEN) aecf5000 - aecf6000 (ACPI NVS) > (XEN) aecf6000 - aed24000 (reserved) > (XEN) aed24000 - aef2f000 (ACPI NVS) > (XEN) aef2f000 - aefed000 (ACPI data) > (XEN) aefed000 - af00 (usable) > (XEN) af00 - b000 (reserved) > (XEN) f800 - fc00 (reserved) > (XEN) fec0 - fec01000 (reserved) > (XEN) fed19000 - fed1a000 (reserved) > (XEN) fed1c000 - fed2 (reserved) > (XEN) fee0 - fee01000 (reserved) > (XEN) ff40 - 0001 (reserved) > (XEN) 0001 - 00085000 (usable) > (XEN) Kdump: DISABLED (failed to reserve 192MB (196608kB) at 0xa020) > (XEN) ACPI: RSDP 000F0410, 0024 (r2 INTEL ) > > When booting with Grub2 capable of locating Xen at its preferred > location, the calculation for the kexec region fails, because the > current location of Xen isn't taken into account. > > The end of the chosen kexec region (0xa020 + 0x0c000000) overlaps > with the bottom of the Xen image. Never got to upstream this, nor actually do the RFC thing I mentioned.. commit 0350412917e7465fe5aaa3ba7616cf9bc6daa533 Author: Konrad Rzeszutek Wilk Date: Wed Dec 16 16:05:31 2015 -0500 kexec/relocate: Change kexec location if relocation is in the way. The issue at hand is that GRUB2 puts us at the end of the E820_RAM that is under 4GB. It is aligned as well. For example : (XEN) Xen image base address: 0xeec0 .. (XEN) Xen-e820 RAM map: (XEN) - 0009fc00 (usable) (XEN) 0009fc00 - 000a (reserved) (XEN) 000f - 0010 (reserved) (XEN) 0010 - e000 (usable) (XEN) e000 - f000 (reserved) If the user decides to put the kexec crashkernel in the same area (so at the end of the E820_RAM) the relocation routines go haywire. For example with " crashkernel=512M@3327M" we would be usurping the end of the E820_RAM. This code doesn't actually fix the underlaying issue with the relocation routines (See below for explanation). Instead it just recomputes the location of where the kexec image should reside. With this patch we will have: (XEN) Kdump: 3327MB-3839MB overlaps with 0xeee0(3822MB). Adjusting. (XEN) Kdump: 512MB (524288kB) at 0xcee0->0xeee0 (XEN) New Xen image base address: 0xef80 The code assumes that the "new" relocation physical is always going to _after_ where GRUB has put the initial code. In other words - we always move it upwards in memory. But in this case there is no space (because kexec has grabbed it all) so we must move it downward (below where GRUB put us). That means subtracting the delta.. But since the value is an unsigned int that negative bit becomes 0xfff instead of -. Then the addition in the pagetables becomes quite large. However an RFC patch that fixes this didn't work right. OraBug: 22371
Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute
On Wed, Nov 29, 2017 at 08:35:33AM -0700, Jan Beulich wrote:
> >>> On 29.11.17 at 16:08, wrote:
> > On 11/9/2017 2:28 AM, Jan Beulich wrote:
> > On 08.11.17 at 16:44, wrote:
> >>> On 11/7/2017 8:40 AM, Jan Beulich wrote:
> >>> On 06.11.17 at 18:48, wrote:
> > --- a/Documentation/ABI/testing/sysfs-driver-pciback
> > +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> > @@ -11,3 +11,15 @@ Description:
> >#echo 00:19.0-E0:2:FF >
> > /sys/bus/pci/drivers/pciback/quirks
> >will allow the guest to read and write to the
> > configuration
> >register 0x0E.
> > +
> > +What: /sys/bus/pci/drivers/pciback/do_flr
> > +Date: Nov 2017
> > +KernelVersion: 4.15
> > +Contact:xen-devel@lists.xenproject.org
> > +Description:
> > +An option to perform a slot or bus reset when a PCI
> > device
> > + is owned by Xen PCI backend. Writing a string of
> > :BB:DD.F
> > + will cause the pciback driver to perform a slot or bus
> > reset
> > + if the device supports it. It also checks to make sure
> > that
> > + all of the devices under the bridge are owned by Xen PCI
> > + backend.
> Why do you name this "do_flr" when you don't even try FLR, but
> go to slot or then bus reset right away.
> >>> Yes, I agree but xen toolstack has already been modified to
> >>> consume"do_flr" attribute. Hence, we are using the
> >>> function that matches with sysfs attribute.
> >> That's not a valid reason imo: Right now the driver doesn't expose
> >> such an attribute, so if the tool stack was trying to use it, it would
> >> not do what is intended at present anyway (i.e. the code could at
> >> best be called dead).
> > Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus
> > reset".
> > Please let me knowyour preference.
>
> Well, that's more a question to Konrad as the maintainer.
> Personally I'd prefer just "reset", as "pci" is redundant and "bus"
Can't do 'reset'.
> doesn't cover the slot variant.
'bus_reset' sounds lovely?
>
> >> Furthermore, contrary to what you claim in
> >> your reply to Pasi, I can't see where you try an actual FLR first -
> >> you go straight to pci_probe_reset_{slot,bus}(). If you actually
> >> tried FLR first, only falling back to the other methods as "emulation",
> >> I could certainly agree with the file name chosen.
> > Currently, multiple resets are being invoked (independently) in the context
> > of "xl attach/detach/shutdown/reboot".
> >
> > - pci_reset_function_locked (invoked by pcistub_put_pci_dev())
> >This function tries various PCI reset methods including FLR.
> > - pcistub_reset_dev (called by toolsstack based on "do_flr" attribute)
>
> While related in a certain way, I can't really see how this addresses
> the comment.
>
> Jan
>
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] Xen/pciback: Implement PCI slot or bus reset with 'do_flr' SysFS attribute
On Wed, Nov 29, 2017 at 09:40:20AM -0700, Jan Beulich wrote: > >>> On 29.11.17 at 17:29, wrote: > > On Wed, Nov 29, 2017 at 08:35:33AM -0700, Jan Beulich wrote: > >> >>> On 29.11.17 at 16:08, wrote: > >> > On 11/9/2017 2:28 AM, Jan Beulich wrote: > >> > On 08.11.17 at 16:44, wrote: > >> >>> On 11/7/2017 8:40 AM, Jan Beulich wrote: > >> >>> On 06.11.17 at 18:48, wrote: > >> > --- a/Documentation/ABI/testing/sysfs-driver-pciback > >> > +++ b/Documentation/ABI/testing/sysfs-driver-pciback > >> > @@ -11,3 +11,15 @@ Description: > >> >#echo 00:19.0-E0:2:FF > > > /sys/bus/pci/drivers/pciback/quirks > >> >will allow the guest to read and write to the > > configuration > >> >register 0x0E. > >> > + > >> > +What: /sys/bus/pci/drivers/pciback/do_flr > >> > +Date: Nov 2017 > >> > +KernelVersion: 4.15 > >> > +Contact:xen-devel@lists.xenproject.org > >> > +Description: > >> > +An option to perform a slot or bus reset when a PCI > >> > device > >> > +is owned by Xen PCI backend. Writing a string of > >> > :BB:DD.F > >> > +will cause the pciback driver to perform a slot or bus > >> > reset > >> > +if the device supports it. It also checks to make sure > >> > that > >> > +all of the devices under the bridge are owned by Xen PCI > >> > +backend. > >> Why do you name this "do_flr" when you don't even try FLR, but > >> go to slot or then bus reset right away. > >> >>> Yes, I agree but xen toolstack has already been modified to > >> >>> consume"do_flr" attribute. Hence, we are using the > >> >>> function that matches with sysfs attribute. > >> >> That's not a valid reason imo: Right now the driver doesn't expose > >> >> such an attribute, so if the tool stack was trying to use it, it would > >> >> not do what is intended at present anyway (i.e. the code could at > >> >> best be called dead). > >> > Sure, we can consider renaming "do_flr" attribute to "pci reset" or "bus > >> > reset". > >> > Please let me knowyour preference. > >> > >> Well, that's more a question to Konrad as the maintainer. > >> Personally I'd prefer just "reset", as "pci" is redundant and "bus" > > > > Can't do 'reset'. > > Why? B/c I forgot that this attribute is not per device, but on the module sub-directory: /sys/bus/pci/drivers/pciback/do_flr It can be indeed called 'reset'. > > >> doesn't cover the slot variant. > > > > 'bus_reset' sounds lovely? > > Lovely sounding or not, it may end up misleading, and even more so > if - like asked for - FLR would be tried first. Fair enough. Reset should work then. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC] xen/arm: Handling cache maintenance instructions by set/way
.snip.. > The suggested policy is based on the KVM one: > - If we trap a S/W instructions, we enable VM trapping (e.g > HCR_EL2.TVM) to > detect cache being turned on/off, and do a full clean. > - We flush the caches on both caches being turned on and off. > - Once the caches are enabled, we stop trapping VM instructions. > > Doing a full clean will require to go through the P2M and flush the entries > one by one. At the moment, all the memory is mapped. As you can imagine > flushing guest with hundreds of MB will take a very long time (Linux timeout > during CPU bring). Yikes. Since you mention 'based on the KVM one' - did they solve this particular problem or do they also have the same issue? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations
> diff --git a/docs/misc/livepatch.pandoc b/docs/misc/livepatch.pandoc
> index 6ab7f4c2d2..92a424e918 100644
> --- a/docs/misc/livepatch.pandoc
> +++ b/docs/misc/livepatch.pandoc
> @@ -300,6 +300,7 @@ which describe the functions to be patched:
> /* Added to livepatch payload version 2: */
> uint8_t applied;
> uint8_t _pad[7];
> +livepatch_expectation_t expect;
> };
>
> The size of the structure is 64 bytes on 64-bit hypervisors. It will be
^^
I also updated this to be 104 and 92 (for 32-bit).
And added:
> @@ -336,6 +337,26 @@ The version 2 of the payload adds the following fields
> to the structure:
>* `applied` tracks function's applied/reverted state. It has a boolean type
> either LIVEPATCH_FUNC_NOT_APPLIED or LIVEPATCH_FUNC_APPLIED.
>* `_pad[7]` adds padding to align to 8 bytes.
> + * `expect` is an optional structure containing expected to-be-replaced data
> +(mostly for inline asm patching). The `expect` structure format is:
> +
> +struct livepatch_expectation {
> +uint8_t enabled : 1;
> +uint8_t len : 5;
uint8_t rsv : 2;
To make it clear what the extra two bits in the bit-field should have.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 00/12] livepatch: new features and fixes
On Tue, Aug 27, 2019 at 08:46:12AM +, Pawel Wieczorkiewicz wrote: > This series introduces new features to the livepatch functionality as > briefly discussed during Xen Developer Summit 2019: [a] and [b]. > It also provides a few fixes and some small improvements. > > Main changes in v2: > - added new features to livepatch documentation > - added livepatch tests > - enabled Arm support for [5] > - make .modinfo optional for [11] > - fixed typos I took your patches, redid them per what I had responded on these patches (and squashed your fix for xen_expectations) and stuck them in my branch: http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=shortlog;h=refs/heads/livepatch.aws.v3 There are three extra patches that were needed for me to test on ARM32 - those are known issues and they don't block your patches. I will post them independent of your patches. From my perspective, your patches are good to go. But I believe I need: - the tools folks Ack on the libxc code changes, - and also an Ack from the REST on sysctl.h, - and Julian OK on the ARM32/ARM64 code changes which are tiny, but nonethless are his. Pawel, If I don't get to send them out by the end of the next week - feel free to grab them from my branch tree and repost them as v3. Thank you! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 09/11] swiotlb-xen: simplify cache maintainance
On Fri, Sep 06, 2019 at 10:19:01AM -0400, Boris Ostrovsky wrote: > On 9/6/19 10:01 AM, Christoph Hellwig wrote: > > On Fri, Sep 06, 2019 at 09:52:12AM -0400, Boris Ostrovsky wrote: > >> We need nop definitions of these two for x86. > >> > >> Everything builds now but that's probably because the calls are under > >> 'if (!dev_is_dma_coherent(dev))' which is always false so compiler > >> optimized is out. I don't think we should rely on that. > > That is how a lot of the kernel works. Provide protypes only for code > > that is semantically compiled, but can't ever be called due to > > IS_ENABLED() checks. It took me a while to get used to it, but it > > actually is pretty nice as the linker does the work for you to check > > that it really is never called. Much better than say a BUILD_BUG_ON(). > > > (with corrected Juergen's email) > > I know about IS_ENABLED() but I didn't realize that this is allowed for > compile-time inlines and such as well. > > Anyway, for non-ARM bits > > Reviewed-by: Boris Ostrovsky Acked-by: Konrad Rzeszutek Wilk as well. Albeit folks have tested this under x86 Xen with 'swiotlb=force' right? I can test it myself but it will take a couple of days. > > If this goes via Xen tree then the first couple of patches need an ack > from ARM maintainers. > > -boris ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [ANNOUNCE] Xen 4.13 Development Update
> == Hypervisor == > > * Per-cpu tasklet > - XEN-28 > - Konrad Rzeszutek Wilk I haven't gotten to them since the posting three years ago? I don't think I will get to them anytime soom too :-( Would love if someone took them over.. P.S: http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=shortlog;h=refs/heads/percpu_tasklet.v4.8.v2 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 10/12] livepatch: Handle arbitrary size names with the list operation
On Tue, Sep 17, 2019 at 08:55:22AM +, Wieczorkiewicz, Pawel wrote:
>
>
> > On 17. Sep 2019, at 10:48, Jan Beulich wrote:
> >
> > On 17.09.2019 10:40, Wieczorkiewicz, Pawel wrote:
> >>
> >>
> >> On 17. Sep 2019, at 10:27, Jan Beulich
> >> mailto:jbeul...@suse.com>> wrote:
> >>
> >> On 16.09.2019 12:59, Pawel Wieczorkiewicz wrote:
> >> @@ -951,11 +952,13 @@ struct xen_sysctl_livepatch_list {
> >> amount of payloads and
> >> version.
> >> OUT: How many payloads left.
> >> */
> >>uint32_t pad; /* IN: Must be zero. */
> >> +uint64_t name_total_size; /* OUT: Total size of all
> >> transfer names */
> >>
> >> Why uint64_t and not uint32_t? You don't expect this to grow
> >> beyond 4GiB, do you?
> >>
> >> I don’t, but uint32_t is not really compatible with size_t.
> >> And I was thought to always use size_t compatible types for sizes.
> >
> > That's a fair point, but I think we use 32-bit sizes elsewhere
> > as well, when crossing the 4GiB boundary would seem entirely
> > unexpected.
> >
> > But what's worse here - you shouldn't use plain uint64_t in
> > sysctl.h (and domctl.h) anyway. If anything, you ought to use
> > uint64_aligned_t. (Going through the file I notice a few other
> > bad instances have crept in.)
> >
>
> I see. Noted, thanks for letting me know.
>
> >> Anyway, I do not mind changing this to whatever type you prefer.
> >
> > Well, preference - if anyone's - would be the livepatch maintainers'
> > one here.
> >
>
> Waiting for the maintainers' wise judgment then :-).
32-bit please. Thx
>
> > Also - can you please see about adjusting your reply style? In plain
> > text mode it's impossible to tell context from your responses.
>
> Oh, sorry about that. I tweaked my settings.
> Please let me know if it does not get better.
>
> >
> > Jan
>
> Best Regards,
> Pawel Wieczorkiewicz
>
>
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
>
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [GIT PULL] (swiotlb) stable/for-linus-5.2
Hi Linus, Please git pull the following branch: git pull git://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git stable/for-linus-5.2 which has cleanups in the swiotlb code and extra debugfs knobs to help with the field diagnostics. Thank you! Christoph Hellwig (4): swiotlb-xen: make instances match their method names swiotlb-xen: use ->map_page to implement ->map_sg swiotlb-xen: simplify the DMA sync method implementations swiotlb-xen: ensure we have a single callsite for xen_dma_map_page Dongli Zhang (2): swiotlb: dump used and total slots when swiotlb buffer is full swiotlb: save io_tlb_used to local variable before leaving critical section drivers/xen/swiotlb-xen.c | 196 ++ kernel/dma/swiotlb.c | 6 +- 2 files changed, 64 insertions(+), 138 deletions(-) signature.asc Description: PGP signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [stable] xen/pciback: Don't disable PCI_COMMAND on PCI device reset.
On 5/30/19 8:16 AM, Ben Hutchings wrote: I'm looking at CVE-2015-8553 which is fixed by: commit 7681f31ec9cdacab4fd10570be924f2cef6669ba Author: Konrad Rzeszutek Wilk Date: Wed Feb 13 18:21:31 2019 -0500 xen/pciback: Don't disable PCI_COMMAND on PCI device reset. I'm aware that this change is incompatible with qemu < 2.5, but that's now quite old. Do you think it makes sense to apply this change to some stable branches? Ben. Hey Ben, My opinion is to drop it, but if Juergen thinks it makes sense to backport I am not going to argue. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen-blkfront: switch kcalloc to kvcalloc for large array allocation
On May 31, 2019 10:41:16 AM EDT, Juergen Gross wrote: >On 06/05/2019 10:11, Juergen Gross wrote: >> On 03/05/2019 17:04, Roger Pau Monne wrote: >>> There's no reason to request physically contiguous memory for those >>> allocations. >>> >>> Reported-by: Ian Jackson >>> Signed-off-by: Roger Pau Monné >> >> Reviewed-by: Juergen Gross > >Jens, are you going to tkae this patch or should I carry it through the >Xen tree? Usually I ended up picking them (and then asking Jens to git pull into his branch) but if you want to handle them that would be much easier! (And if so, please add Acked-by on them from me). > > >Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/swiotlb: don't initialize swiotlb twice on arm64
On Tue, Jun 04, 2019 at 03:41:40PM -0400, Boris Ostrovsky wrote:
> On 6/4/19 12:51 PM, Stefano Stabellini wrote:
> > On Mon, 3 Jun 2019, Boris Ostrovsky wrote:
> >> On 6/3/19 2:25 PM, Stefano Stabellini wrote:
> >>> On Tue, 28 May 2019, Boris Ostrovsky wrote:
> On 5/28/19 6:48 PM, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > On arm64 swiotlb is often (not always) already initialized by mem_init.
> > We don't want to initialize it twice, which would trigger a second
> > memory allocation. Moreover, the second memory pool is typically made of
> > high pages and ends up replacing the original memory pool of low pages.
> > As a side effect of this change, it is possible to have low pages in
> > swiotlb-xen on arm64.
> >
> > Signed-off-by: Stefano Stabellini
> Has this been tested on x86?
> >>> Yes, I managed to test it using QEMU. There are no effects on x86, as
> >>> the check io_tlb_start != 0 returns false.
> >> I wonder though whether this is always the case. When we are called
> >> from pci_xen_swiotlb_init_late() for example.
> > In that case, pci_xen_swiotlb_init_late() is called by
> > pcifront_connect_and_init_dma, which does:
> >
> > if (!err && !swiotlb_nr_tbl()) {
> > err = pci_xen_swiotlb_init_late();
> > if (err)
> > dev_err(&pdev->xdev->dev, "Could not setup SWIOTLB!\n");
> > }
> >
> > pci_xen_swiotlb_init_late() is only called when swiotlb_nr_tbl() returns
> > 0. If swiotlb_nr_tbl() returns 0, certainly the swiotlb has not been
> > allocated yet, and the io_tlb_start != 0 check at the beginning of
> > xen_swiotlb_init will also fail. The code will take the normal
> > route, same as today. In short, there should be no effects on x86.
>
>
> OK, thanks.
>
> Reviewed-by: Boris Ostrovsky
Pushed in devel/for-linus-5.2 and will eventually move it to stable and push to
Linus next-week.
Are there any other patches I should pick up?
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 0/3] Remove tmem
On Mon, Dec 31, 2018 at 12:43:39PM +, Andrew Cooper wrote:
> On 03/12/2018 09:56, Jan Beulich wrote:
> On 30.11.18 at 19:01, wrote:
> >> On Fri, Nov 30, 2018 at 05:09:42PM +, Ian Jackson wrote:
> >>> Wei Liu writes ("[PATCH v2 0/3] Remove tmem"):
> It is agreed that tmem can be removed from xen.git. See the thread
> starting
> >>
> >>
> >>
> from .
> >>> Those are notes from some phone call amongst industry stakeholders.
> >>> None of the messages have a Subject line mentioning tmem. There is no
> >>> explanation of the basis for the decision; just a confirmation from
> >>> the current maintainers that they will ack the removal.
> >>>
> >>> I think this is not really an appropriate way to carry on! What if
> >>> there is someone else who wants to step up to maintain this ? What
> >>> about user communication ? Going straight from `Supported' to
> >>> `Deleted' seems rather vigorous.
> >> Step up to maintain> I would rather say step up to develop.
> >>
> >> The status in MAINTAINERS is wrong. According to SUPPORT.md, it is only
> >> experimental. Our definition of "experimental" is:
> >>
> >>Functional completeness: No
> >>Functional stability: Here be dragons
> >>Interface stability: Not stable
> >>Security supported: No
> > Exactly. Plus my proposal to remove it was posted to xen-devel
> > on Aug 30th. I don't think removal of an experimental feature
> > requires posting to xen-announce. Ian - please reconsider your
> > nack.
>
> I concur with Wei and Jan. TMEM has been off by default due to being
> declared "full of security holes - don't use" since XSA-15. That was in
> 2012, and TMEM hasn't made its way back into security support in that time.
>
> In addition, it was never fixed to work with Migration v2. The save
> side doesn't query any TMEM state, and convert-legacy-stream raises TODO
> on encountering legacy TMEM data.
>
> I don't know about other distributions, but it has been compiled out of
> XenServer for all versions which have Kconfig.
>
> tl;dr It doesn't work, and at this point, it looks very unlikely to
> change. There is a non-zero cost for retaining obsolete functionality,
> and the hypervisor maintainers want it gone in 4.12, which we think is
> entirely reasonable given the circumstances.
I agree on all counts. Can we please remove it?
>
> ~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] tmem: default to off
On Wed, Jan 09, 2019 at 05:16:17PM +, Wei Liu wrote: > On Wed, Jan 09, 2019 at 02:05:19AM -0700, Jan Beulich wrote: > > As a short term alternative to deleting the code, default its building > > to off (overridable in EXPERT mode only). Additionally make sure other > > related baggage (LZO code) won't be carried when the option is off (with > > TMEM scheduled to be deleted anyway, I didn't want to introduce a > > separate Kconfig option to control the LZO compression code, and hence > > CONFIG_TMEM is used directly there). Similarly I couldn't be bothered to > > add actual content to the command line option doc for the two affected > > options. > > > > Signed-off-by: Jan Beulich > > Acked-by: Wei Liu Acked-by: Konrad Rzeszutek Wilk Thank you! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront
On Tue, Jan 08, 2019 at 04:24:32PM +0800, Dongli Zhang wrote: > oops. Please ignore this v5 patch. > > I just realized Linus suggested in an old email not use BUG()/BUG_ON() in the > code. > > I will switch to the WARN() solution and resend again. OK. Did I miss it? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [OSSTEST PATCH 2/3] ts-livepatch-run: Print a message about expected failures
On Wed, Jan 16, 2019 at 11:36:36AM +, Ian Jackson wrote:
> target_cmd_output_root_status prints the command exit status. If that
> was a failure and the failure was as expected, this can be confusing
> to readers who do not know that this is a possibility. So print a
> message about it.
>
> Signed-off-by: Ian Jackson
> CC: Konrad Rzeszutek Wilk
Reviewed-by: Konrad Rzeszutek Wilk
Thank you!
> ---
> ts-livepatch-run | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ts-livepatch-run b/ts-livepatch-run
> index 8fdb8004..f011e64e 100755
> --- a/ts-livepatch-run
> +++ b/ts-livepatch-run
> @@ -152,6 +152,9 @@ sub livepatch_test () {
> die "FAILED! OutputCheck=$test->{OutputCheck},
> input=$output\n";
> }
> }
> + if ($expected_rc) {
> + logm "... that failure ($rc) was as required; all is well.";
> +}
> }
> return 0;
> }
> --
> 2.11.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [OSSTEST PATCH 3/3] ts-livepatch-run: Fix erroneous $$ in double-check
On Wed, Jan 16, 2019 at 11:36:37AM +, Ian Jackson wrote:
> The doubled $s here are simply a mistake. The result is to make this
> test ineffective, since `$$file' means `the value of the variable
> whose name is in the variable $file', which here will never exist.
> This produces a `Use of uninitialized value' warning and substitutes
> the empty string, so overall we test the existence of the directory.
>
> The missing check is not of much consequence since this check is not
> really expected ever to fail, and if it does, some actual test
> execution would fail due to the missing file.
>
> So overall I think the only change is to log output.
>
> Signed-off-by: Ian Jackson
> CC: Konrad Rzeszutek Wilk
Reviewed-by: Konrad Rzeszutek Wilk
Thank you!
> ---
> ts-livepatch-run | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ts-livepatch-run b/ts-livepatch-run
> index f011e64e..86a79791 100755
> --- a/ts-livepatch-run
> +++ b/ts-livepatch-run
> @@ -161,7 +161,7 @@ sub livepatch_test () {
>
> sub livepatch_check () {
> foreach my $file (@livepatch_files) {
> -if (!target_file_exists($ho, "/usr/lib/debug/xen-livepatch/$$file"))
> {
> +if (!target_file_exists($ho, "/usr/lib/debug/xen-livepatch/$file")) {
> die "$file is missing!\n";
> }
> }
> --
> 2.11.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [OSSTEST PATCH 1/3] ts-livepatch-run: Treat (just) falseish from OutputCheck as fail
On Wed, Jan 16, 2019 at 11:36:35AM +, Ian Jackson wrote:
> This is more idiomatic. All existing OutputChecks return booleans, so
> no functional change.
>
> Signed-off-by: Ian Jackson
> CC: Konrad Rzeszutek Wilk
Reviewed-by: Konrad Rzeszutek Wilk
Thank you!
> ---
> ts-livepatch-run | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ts-livepatch-run b/ts-livepatch-run
> index f1194586..8fdb8004 100755
> --- a/ts-livepatch-run
> +++ b/ts-livepatch-run
> @@ -147,8 +147,8 @@ sub livepatch_test () {
> }
> if (defined($test->{OutputCheck})) {
> $_ = $output;
> -$rc=$test->{OutputCheck}->();
> -if ($rc ne 1) {
> +my $ok = $test->{OutputCheck}->();
> +if (!$ok) {
> die "FAILED! OutputCheck=$test->{OutputCheck},
> input=$output\n";
> }
> }
> --
> 2.11.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 2/2] xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront
On Tue, Jan 15, 2019 at 09:20:36AM +0100, Roger Pau Monné wrote: > On Tue, Jan 15, 2019 at 12:41:44AM +0800, Dongli Zhang wrote: > > The xenstore 'ring-page-order' is used globally for each blkback queue and > > therefore should be read from xenstore only once. However, it is obtained > > in read_per_ring_refs() which might be called multiple times during the > > initialization of each blkback queue. > > > > If the blkfront is malicious and the 'ring-page-order' is set in different > > value by blkfront every time before blkback reads it, this may end up at > > the "WARN_ON(i != (XEN_BLKIF_REQS_PER_PAGE * blkif->nr_ring_pages));" in > > xen_blkif_disconnect() when frontend is destroyed. > > > > This patch reworks connect_ring() to read xenstore 'ring-page-order' only > > once. > > > > Signed-off-by: Dongli Zhang > > LGTM: > > Reviewed-by: Roger Pau Monné Applied. Will push out to Jens in a couple of days. Thank you! > > Thanks! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 00/27] x86: PIE support and option to extend KASLR randomization
On Thu, Jan 31, 2019 at 11:24:07AM -0800, Thomas Garnier wrote: > There has been no major concern in the latest iterations. I am interested on > what would be the best way to slowly integrate this patchset upstream. One question that I was somehow expected in this cover letter - what about all those lovely speculative bugs? As in say some one hasn't updated their machine with the Spectre v3a microcode - wouldn't they be able to get the kernel virtual address space? In effect rendering all this hard-work not needed? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch-hooks-2 PATCH 2/4] create-diff-object: Add support for applied/reverted marker
On 8/14/19 4:38 AM, Pawel Wieczorkiewicz wrote:
With version 2 of a payload structure additional field is supported
to track whether given function has been applied or reverted.
There also comes additional 8-byte alignment padding to reserve
place for future flags and options.
The new fields are zero-out upon .livepatch.funcs section creation.
Signed-off-by: Pawel Wieczorkiewicz
---
common.h | 2 ++
create-diff-object.c | 4 +++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/common.h b/common.h
index 06e19e7..d8cde35 100644
--- a/common.h
+++ b/common.h
@@ -124,6 +124,8 @@ struct livepatch_patch_func {
uint32_t old_size;
uint8_t version;
unsigned char pad[31];
So the 31 pad is for this purpose - that you can make it smaller. Why
not use that?
+ uint8_t applied;
+ uint8_t _pad[7];
};
struct special_section {
diff --git a/create-diff-object.c b/create-diff-object.c
index 263c7d2..534516b 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -2009,8 +2009,10 @@ static void livepatch_create_patches_sections(struct
kpatch_elf *kelf,
funcs[index].old_size = result.size;
funcs[index].new_addr = 0;
funcs[index].new_size = sym->sym.st_size;
- funcs[index].version = 1;
+ funcs[index].version = 2;
memset(funcs[index].pad, 0, sizeof funcs[index].pad);
+ funcs[index].applied = 0;
+ memset(funcs[index]._pad, 0, sizeof funcs[index]._pad);
/*
* Add a relocation that will populate
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch-hooks-2 PATCH 3/4] livepatch: Add support for apply|revert action replacement hooks
On 8/14/19 4:38 AM, Pawel Wieczorkiewicz wrote: By default, in the quiescing zone, a hotpatch payload is applied with apply_payload() and reverted with revert_payload() functions. Both of the functions receive the payload struct pointer as a parameter. The functions are also a place where standard 'load' and 'unload' module hooks are executed. To increase hotpatching system's agility and provide more flexiable long-term hotpatch solution, allow to overwrite the default apply and revert action functions with hook-like supplied alternatives. The alternative functions are optional and the default functions are used by default. Since the alternative functions have direct access to the hotpatch payload structure, they can better control context of the 'load' and 'unload' hooks execution as well as exact instructions replacement workflows. They can be also easily extended to support extra features in the future. To simplify the alternative function generation move code responsible for payload and hotpatch region registration outside of the function. That way it is guaranteed that the registration step occurs even for newly supplied functions. You MUST also include the test-cases for this new functionality. Please add them, you know where they are right? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch-hooks-2 PATCH 4/4] livepatch: Add per-function applied/reverted state tracking marker
On 8/14/19 4:39 AM, Pawel Wieczorkiewicz wrote:
#ifdef __XEN__
+typedef enum livepatch_func_state {
+LIVEPATCH_FUNC_NOT_APPLIED = 0,
+LIVEPATCH_FUNC_APPLIED = 1
+} livepatch_func_state_t;
+
struct livepatch_func {
const char *name; /* Name of function to be patched. */
void *new_addr;
@@ -834,6 +839,10 @@ struct livepatch_func {
uint32_t old_size;
uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
uint8_t opaque[31];
+#if defined CONFIG_X86
+uint8_t applied;
+uint8_t _pad[7];
+#endif
};
Three requests:
- Why does it have to be X86 specific?
- Can you also include the change in the documentation where the spec
resides?
- You are bumping the version to 2, but not making it backwards
compatible. If so ,you also need to update the documentation to mention
this.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 01/14] livepatch: Always check hypervisor build ID upon hotpatch upload
On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote: This change is part of a independant stacked hotpatch modules feature. This feature allows to bypass dependencies between modules upon loading, but still verifies Xen build ID matching. In order to prevent (up)loading any hotpatches built for different hypervisor version as indicated by the Xen Build ID, add checking for the payload's vs Xen's build id match. To achieve that embed into every hotpatch another section with a dedicated hypervisor build id in it. After the payload is loaded and the .livepatch.xen_depends section becomes available, perform the check and reject the payload if there is no match. Signed-off-by: Pawel Wieczorkiewicz Reviewed-by: Andra-Irina Paraschiv Reviewed-by: Bjoern Doebel Reviewed-by: Eslam Elnikety Reviewed-by: Martin Pohlack +# This one always fails upon upload, because it deliberetely I think that is spelled a bit different :-) But besides that looks perfect. Ross, you Ok with this one too? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 09/14] livepatch: Add per-function applied/reverted state tracking marker
On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote:
struct livepatch_func {
const char *name; /* Name of function to be patched. */
void *new_addr;
@@ -834,6 +839,10 @@ struct livepatch_func {
uint32_t old_size;
uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
uint8_t opaque[31];
+#if defined CONFIG_X86
+uint8_t applied;
+uint8_t _pad[7];
+#endif
Replying here as well. Could you use part of the 'opaque[31]' space
instead for this field?
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 10/14] livepatch: Add support for inline asm hotpatching expectations
On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote:
typedef enum livepatch_func_state {
LIVEPATCH_FUNC_NOT_APPLIED = 0,
LIVEPATCH_FUNC_APPLIED = 1
@@ -838,11 +850,12 @@ struct livepatch_func {
uint32_t new_size;
uint32_t old_size;
uint8_t version;/* MUST be LIVEPATCH_PAYLOAD_VERSION. */
-uint8_t opaque[31];
+uint8_t opaque[LIVEPATCH_OPAQUE_SIZE];
#if defined CONFIG_X86
uint8_t applied;
uint8_t _pad[7];
#endif
+livepatch_expectation_t expect;
};
Aaah, now I understand why you decide to create a new field _pad and
applied field!
Any particular reason why the version can't be 2 since this can be part
of this changeset? Also you would need to have a Documentation change.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 06/14] livepatch: Add support for apply|revert action replacement hooks
On 8/21/19 4:19 AM, Pawel Wieczorkiewicz wrote: By default, in the quiescing zone, a hotpatch payload is applied with apply_payload() and reverted with revert_payload() functions. Both of the functions receive the payload struct pointer as a parameter. The functions are also a place where standard 'load' and 'unload' module hooks are executed. To increase hotpatching system's agility and provide more flexiable long-term hotpatch solution, allow to overwrite the default apply and revert action functions with hook-like supplied alternatives. The alternative functions are optional and the default functions are used by default. Since the alternative functions have direct access to the hotpatch payload structure, they can better control context of the 'load' and 'unload' hooks execution as well as exact instructions replacement workflows. They can be also easily extended to support extra features in the future. To simplify the alternative function generation move code responsible for payload and hotpatch region registration outside of the function. That way it is guaranteed that the registration step occurs even for newly supplied functions. This logic looks legit, but I was wondering if you would be up for a writing a small test-case(s) livepatch that would exercise these parts just to make sure nothing in the future would screw it up? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH lp-metadata 2/3] livepatch: Handle arbitrary size names with the list operation
On 8/15/19 12:29 PM, Andrew Cooper wrote: On 15/08/2019 16:42, Wieczorkiewicz, Pawel wrote: Thanks Julien. I will do that next time (unless you guys want me to re-send all this ;-)). BTW, I also pushed my changes onto the xenbits server: http://xenbits.xenproject.org/gitweb/?p=people/wipawel/livepatch-build-tools;a=summary http://xenbits.xenproject.org/gitweb/?p=people/wipawel/xen;a=summary I hope that makes navigation and dealing with the swarm of patches a bit easier. Please (re)send two patch series, one for Xen and one for build tools. Even for he subset you posted before, I can't figure out whether they're ok to push straight away, or need more review. This will be far easier to do in one single go (per repo). They look pretty good. Just need some extra test-cases. And I will need to update the ts-livepatch test-case to take advantage of them. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 08/20] livepatch-build: detect special section group sizes
+# Using xen-syms built in the previous step by build_full(). +SPECIAL_VARS=$(readelf -wi "$OUTPUT/xen-syms" | What version of readelf supports this? Asking as in the past there were some options with binutils that had conflicting options and we had to add some custom hackery code to figure out which parameter to use. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 06/14] livepatch: Add support for apply|revert action replacement hooks
> Yes, I could do that. But, I would like to discuss (get guidelines about) the > expected test coverage. > With this sort of changes, there is an unlimited set of test-cases to be > created. So, I would like to focus on a few most important. > I am missing knowledge how these test cases are supposed to be used… hence > the ask. I had in mind two livepatches with a dependency on each other that would utilize different callback mechanism. Say the first one just modifies xen_extra_function to print based on a string variable. Applying it would change extra version to say 'FIRST_LIVEPATCH' or such. The second one has an apply and revert callback that modifies the xen_extra_function to print say 'SECOND_LIVEPATCH' and also 'SECOND_REVERTED' when reverted? So you would have: xen-livepatch apply xen_first xl version |grep extra_version [should have FIRST_LIVEPATCH] xen-livepatch apply xen_second xl version |grep extra_version [should have SECOND_LIVEPATCH] xen-livepatch revert xen_second xl version |grep extra_version [should have SECOND_REVERTED] or such? Just want to make sure that the code that is doing the various combinations of callbacks is behaving correctly. Naturally doing all of them would be rather difficult, so I would rather test the most common use-case. Hope this helps? > > Best Regards, > Pawel Wieczorkiewicz > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B > Sitz: Berlin > Ust-ID: DE 289 237 879 > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] livepatch: Identify the object file create-diff-object dislikes
On August 27, 2019 11:38:39 AM EDT, Andrew Cooper
wrote:
>... rather than leaving the user with no hint as to where to debug
>next.
>
>Signed-off-by: Andrew Cooper
>---
>CC: Konrad Rzeszutek Wilk
Reviewed-by: Konrad Rzeszutek Wilk
>CC: Ross Lagerwall
>---
> livepatch-build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/livepatch-build b/livepatch-build
>index 7068faf..b198c97 100755
>--- a/livepatch-build
>+++ b/livepatch-build
>@@ -140,7 +140,7 @@ function create_patch()
>die "no core file found, run 'ulimit -c unlimited' and try to recreate"
> fi
># create-diff-object returns 3 if no functional change is found
>-[[ $rc -eq 0 ]] || [[ $rc -eq 3 ]] || ERROR=$(expr $ERROR "+"
>1)
>+[[ $rc -eq 0 ]] || [[ $rc -eq 3 ]] || { ERROR=$(expr $ERROR
>"+" 1); warn "create-diff-object $i rc $rc"; }
> if [[ $rc -eq 0 ]]; then
> CHANGED=1
> fi
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 05/12] livepatch: Add support for apply|revert action replacement hooks
On August 27, 2019 4:46:17 AM EDT, Pawel Wieczorkiewicz wrote: >By default, in the quiescing zone, a hotpatch payload is applied with >apply_payload() and reverted with revert_payload() functions. Both of >the functions receive the payload struct pointer as a parameter. The >functions are also a place where standard 'load' and 'unload' module >hooks are executed. > >To increase hotpatching system's agility and provide more flexiable >long-term hotpatch solution, allow to overwrite the default apply >and revert action functions with hook-like supplied alternatives. >The alternative functions are optional and the default functions are >used by default. Is there sense in having the old ones anymore? We could just remove them altogether and just use the new ones instead from the start? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations
> diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
> index 23113d3418..067861903f 100644
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
> @@ -27,6 +27,8 @@ LIVEPATCH_ACTION_HOOKS_NOFUNC :=
> xen_action_hooks_nofunc.livepatch
> LIVEPATCH_ACTION_HOOKS_MARKER:= xen_action_hooks_marker.livepatch
> LIVEPATCH_ACTION_HOOKS_NOAPPLY:= xen_action_hooks_noapply.livepatch
> LIVEPATCH_ACTION_HOOKS_NOREVERT:= xen_action_hooks_norevert.livepatch
> +LIVEPATCH_EXPECTATIONS:= xen_expectations.livepatch
> +LIVEPATCH_EXPECTATIONS_FAIL:= xen_expectations_fail.livepatch
>
> LIVEPATCHES += $(LIVEPATCH)
> LIVEPATCHES += $(LIVEPATCH_BYE)
> @@ -40,6 +42,8 @@ LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOFUNC)
> LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_MARKER)
> LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOAPPLY)
> LIVEPATCHES += $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
> +LIVEPATCHES += $(LIVEPATCH_EXPECTATIONS)
> +LIVEPATCHES += $(LIVEPATCH_EXPECTATIONS_FAIL)
>
> LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
>
> @@ -54,7 +58,7 @@ uninstall:
>
> .PHONY: clean
> clean::
> - rm -f *.o .*.o.d *.livepatch config.h
> + rm -f *.o .*.o.d *.livepatch config.h expect_config.h
>
> #
> # To compute these values we need the binary files: xen-syms
> @@ -182,8 +186,27 @@ xen_actions_hooks_norevert.o: config.h
> $(LIVEPATCH_ACTION_HOOKS_NOREVERT): xen_action_hooks_marker.o
> xen_hello_world_func.o note.o xen_note.o
> $(LD) $(LDFLAGS) $(build_id_linker) -r -o
> $(LIVEPATCH_ACTION_HOOKS_NOREVERT) $^
>
> +CODE_GET_EXPECT=$(shell objdump -d --insn-width=1 $(1) | grep -A6 -E
> '<'$(2)'>:' | tail -n +2 | awk 'BEGIN {printf "{"} {printf "0x%s,", $$2}' |
> sed 's/,$$/}/g')
> +.PHONY: expect_config.h
> +expect_config.h: EXPECT_BYTES=$(call
> CODE_GET_EXPECT,$(BASEDIR)/xen-syms,xen_extra_version)
> +expect_config.h: EXPECT_BYTES_COUNT=6
> +expect_config.h: xen_expectations.o
> + (set -e; \
> + echo "#define EXPECT_BYTES $(EXPECT_BYTES)"; \
> + echo "#define EXPECT_BYTES_COUNT $(EXPECT_BYTES_COUNT)") > $@
> +
> +xen_expectations.o: expect_config.h
> +
> +.PHONY: $(LIVEPATCH_EXPECTATIONS)
> +$(LIVEPATCH_EXPECTATIONS): xen_expectations.o xen_hello_world_func.o note.o
> xen_note.o
> + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_EXPECTATIONS) $^
> +
> +.PHONY: $(LIVEPATCH_EXPECTATIONS_FAIL)
> +$(LIVEPATCH_EXPECTATIONS_FAIL): xen_expectations_fail.o
> xen_hello_world_func.o note.o xen_note.o
> + $(LD) $(LDFLAGS) $(build_id_linker) -r -o
> $(LIVEPATCH_EXPECTATIONS_FAIL) $^
> +
> .PHONY: livepatch
> livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE)
> $(LIVEPATCH_NOP) $(LIVEPATCH_NO_XEN_BUILDID) \
> $(LIVEPATCH_PREPOST_HOOKS) $(LIVEPATCH_PREPOST_HOOKS_FAIL)
> $(LIVEPATCH_ACTION_HOOKS) \
> $(LIVEPATCH_ACTION_HOOKS_NOFUNC) $(LIVEPATCH_ACTION_HOOKS_MARKER)
> $(LIVEPATCH_ACTION_HOOKS_NOAPPLY) \
> - $(LIVEPATCH_ACTION_HOOKS_NOREVERT)
> + $(LIVEPATCH_ACTION_HOOKS_NOREVERT) $(LIVEPATCH_EXPECTATIONS)
> $(LIVEPATCH_EXPECTATIONS_FAIL)
> diff --git a/xen/test/livepatch/xen_expectations.c
> b/xen/test/livepatch/xen_expectations.c
> new file mode 100644
> index 00..c8175a458b
> --- /dev/null
> +++ b/xen/test/livepatch/xen_expectations.c
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (c) 2019 Amazon.com, Inc. or its affiliates. All rights
> reserved.
> + *
> + */
> +
> +#include "expect_config.h"
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +static const char livepatch_exceptions_str[] = "xen_extra_version";
> +extern const char *xen_hello_world(void);
> +
> +struct livepatch_func __section(".livepatch.funcs") livepatch_exceptions = {
> +.version = LIVEPATCH_PAYLOAD_VERSION,
> +.name = livepatch_exceptions_str,
> +.new_addr = xen_hello_world,
> +.old_addr = xen_extra_version,
> +.new_size = EXPECT_BYTES_COUNT,
> +.old_size = EXPECT_BYTES_COUNT,
> +.expect = {
> +.enabled = 1,
> +.len = EXPECT_BYTES_COUNT,
> +.data = EXPECT_BYTES
> +},
> +
> +};
When I compile with 32-bit ARM 'make tests' I get:
arm-eabi-ld-EL -EL --build-id=sha1 -r -o
xen_action_hooks_norevert.livepatch xen_action_hooks_marker.o
xen_hello_world_func.o note.o xen_note.o
make[3]: Circular expect_config.h <- xen_expectations.o dependency dropped.
objdump: can't disassemble for architecture UNKNOWN!
(set -e; \
echo "#define EXPECT_BYTES {"; \
echo "#define EXPECT_BYTES_COUNT 6") > expect_config.h
arm-eabi-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall
-Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable
-Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin
-fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__
-include /home/konrad/A20/xen.git/xen/include/xen/config.h
'-D__OBJECT_FILE__="xen_expectations.o"' -Wa,--strip-l
Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations
> +CODE_GET_EXPECT=$(shell objdump -d --insn-width=1 $(1) | grep -A6 -E
> '<'$(2)'>:' | tail -n +2 | awk 'BEGIN {printf "{"} {printf "0x%s,", $$2}' |
> sed 's/,$$/}/g')
Ony my Hikey 960 when I compile using an native compiler I get:
gcc -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
-Wdeclaration-after-statement -Wno-unused-but-set-variable
-Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin
-fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__
-include /home/xen.git/xen/include/xen/config.h
'-D__OBJECT_FILE__="xen_expectations.o"' -Wa,--strip-local-absolute -g -MMD -MF
./.xen_expectations.o.d -mcpu=generic -mgeneral-regs-only
-I/home/xen.git/xen/include -fno-stack-protector -fno-exceptions
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -DBUILD_ID
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
-Wdeclaration-after-statement -Wno-unused-but-set-variable
-Wno-unused-local-typedefs -c xen_expectations.c -o xen_expectations.o
/home/xen.git/xen/Rules.mk:202: recipe for target 'xen_expectations.o' failed
make[3]: Circular expect_config.h <- xen_expectations.o dependency dropped.
In file included from xen_expectations.c:6:0:
expect_config.h:1:23: error: large integer implicitly truncated to unsigned type
[-Werror=overflow]
#define EXPECT_BYTES {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
c09112e0}
^
xen_expectations.c:28:17: note: in expansion of macro ‘EXPECT_BYTES’
.data = EXPECT_BYTES
^~~~
expect_config.h:1:34: error: large integer implicitly truncated to unsigned type
[-Werror=overflow]
#define EXPECT_BYTES {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
c09112e0}
^
xen_expectations.c:28:17: note: in expansion of macro ‘EXPECT_BYTES’
.data = EXPECT_BYTES
^~~~
expect_config.h:1:45: error: large integer implicitly truncated to unsigned type
[-Werror=overflow]
#define EXPECT_BYTES {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
c09112e0}
…
make[3]: Leaving directory '/home/xen.git/xen/test/livepatch'
Makefile:11: recipe for target 'build' failed
make[2]: Leaving directory '/home/xen.git/xen/test'
Makefile:85: recipe for target '_tests' failed
make[1]: Leaving directory '/home/xen.git/xen'
Makefile:45: recipe for target 'tests' failed
root@hikey960:/home/xen.git/xen# cat test/livepatch/expect_config.h
#define EXPECT_BYTES
{0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0xc09112e0}
#define EXPECT_BYTES_COUNT 6
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations
On Thu, Aug 29, 2019 at 04:16:13PM +, Wieczorkiewicz, Pawel wrote:
>
>
> On 29. Aug 2019, at 17:58, Konrad Rzeszutek Wilk
> mailto:konrad.w...@oracle.com>> wrote:
>
> +CODE_GET_EXPECT=$(shell objdump -d --insn-width=1 $(1) | grep -A6 -E
> '<'$(2)'>:' | tail -n +2 | awk 'BEGIN {printf "{"} {printf "0x%s,", $$2}' |
> sed 's/,$$/}/g')
>
> Ony my Hikey 960 when I compile using an native compiler I get:
>
> gcc -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
> -Wdeclaration-after-statement -Wno-unused-but-set-variable
> -Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc
> -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla
> -pipe -D__XEN__ -include /home/xen.git/xen/include/xen/config.h
> '-D__OBJECT_FILE__="xen_expectations.o"' -Wa,--strip-local-absolute -g -MMD
> -MF ./.xen_expectations.o.d -mcpu=generic -mgeneral-regs-only
> -I/home/xen.git/xen/include -fno-stack-protector -fno-exceptions
> -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -DBUILD_ID
> -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
> -Wdeclaration-after-statement -Wno-unused-but-set-variable
> -Wno-unused-local-typedefs -c xen_expectations.c -o xen_expectations.o
> /home/xen.git/xen/Rules.mk:202: recipe for target 'xen_expectations.o' failed
> make[3]: Circular expect_config.h <- xen_expectations.o dependency dropped.
> In file included from xen_expectations.c:6:0:
> expect_config.h:1:23: error: large integer implicitly truncated to unsigned
> type
> [-Werror=overflow]
> #define EXPECT_BYTES
> {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
> c09112e0}
> ^
> xen_expectations.c:28:17: note: in expansion of macro ‘EXPECT_BYTES’
> .data = EXPECT_BYTES
> ^~~~
> expect_config.h:1:34: error: large integer implicitly truncated to unsigned
> type
> [-Werror=overflow]
> #define EXPECT_BYTES
> {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
> c09112e0}
> ^
> xen_expectations.c:28:17: note: in expansion of macro ‘EXPECT_BYTES’
> .data = EXPECT_BYTES
> ^~~~
> expect_config.h:1:45: error: large integer implicitly truncated to unsigned
> type
> [-Werror=overflow]
> #define EXPECT_BYTES
> {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0x
> c09112e0}
> …
>
> make[3]: Leaving directory '/home/xen.git/xen/test/livepatch'
> Makefile:11: recipe for target 'build' failed
> make[2]: Leaving directory '/home/xen.git/xen/test'
> Makefile:85: recipe for target '_tests' failed
> make[1]: Leaving directory '/home/xen.git/xen'
> Makefile:45: recipe for target 'tests' failed
> root@hikey960:/home/xen.git/xen# cat test/livepatch/expect_config.h
> #define EXPECT_BYTES
> {0xf260,0x00f2,0xe000f000,0x12e000f0,0x9112e000,0xc09112e0}
> #define EXPECT_BYTES_COUNT 6
>
>
>
> Could you please try with the patch attached?
It compiled, but the test-case was not happy. See attached the full serial log
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
>
>
>
> If it still fails, could you please send me the output for the following
> command with this build?
>
> objdump -d xen-syms | sed -n -e '/:$/,/^$/ p' | tail -n +2
Also included in the serial log.
>
> Best Regards,
> Pawel Wieczorkiewicz
>
>
>
Loading driver at 0x000B87C8000 EntryPoint=0x000B8870370
Using modules provided by bootloader in FDT
Xen 4.13-unstable (c/s Thu Aug 29 15:43:23 2019 + git:0a1b27af47-dirty) EFI
loader
Xen 4.13-unstable
(XEN) Xen version 4.13-unstable (root@lan) (gcc (Debian 6.3.0-18+deb9u1) 6.3.0
20170516) debug=y Thu Aug 29 17:26:25 UTC 2019
(XEN) Latest ChangeSet: Thu Aug 29 15:43:23 2019 + git:0a1b27af47-dirty
(XEN) build-id: 416315091386424648bd584d25c7224ee5b5d998
(XEN) Processor: 410fd034: "ARM Limited", variant: 0x0, part 0xd03, rev 0x4
(XEN) 64-bit Execution:
(XEN) Processor Features:
(XEN) Exception Levels: EL3:64+32 EL2:64+32 EL1:64+32 EL0:64+32
(XEN) Extensions: FloatingPoint AdvancedSIMD
(XEN) Debug Features: 10305106
(XEN) Auxiliary Features:
(XEN) Memory Model Features: 1122
(XEN) ISA Features: 00011120 0
Re: [Xen-devel] [PATCH v2 00/12] livepatch: new features and fixes
> Pawel Wieczorkiewicz (12):
> [1] livepatch: Always check hypervisor build ID upon hotpatch upload
> [2] livepatch: Allow to override inter-modules buildid dependency
> [3] livepatch: Export payload structure via livepatch_payload.h
> [4] livepatch: Implement pre-|post- apply|revert hooks
> [5] livepatch: Add support for apply|revert action replacement hooks
> [6] livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence
> [7] livepatch: Add per-function applied/reverted state tracking marker
I've added the test-cases to the little tool I use (including the diff)
http://xenbits.xen.org/gitweb/?p=xentesttools/bootstrap.git;a=blob;f=root_image/debugspace/livepatch_test.pl;h=37fb668a53ca1e7a084bfc4417f90e8ae078f7e5;hb=HEAD
> [8] livepatch: Add support for inline asm hotpatching expectations
..but didn't expand on #8 as it still needs a bit of help on ARM..
Irrespective of that:
a) Need to update the docs to alter the text where it says that the
entries are 8 bytes long - as on ARM32 they are 4 bytes.
b) Update the docs to say it is spec 2, not 1.
c) Also need OK or Acked-by from Julie on ARM.
I can do a) and b) when v3 is posted or when the fixes for to patch #8 are
all good and can squash them in. (And will also update the test-case to
include the code change for the asm livepatch).
diff --git a/root_image/debugspace/livepatch_test.pl
b/root_image/debugspace/livepatch_test.pl
index 37fb668..a96c9fc 100755
--- a/root_image/debugspace/livepatch_test.pl
+++ b/root_image/debugspace/livepatch_test.pl
@@ -9,7 +9,14 @@ use File::Temp qw(tempfile);
my @livepatch_files = qw(xen_hello_world.livepatch
xen_replace_world.livepatch
xen_bye_world.livepatch
-xen_nop.livepatch);
+xen_nop.livepatch
+xen_no_xen_buildid.livepatch
+xen_prepost_hooks_fail.livepatch
+xen_prepost_hooks.livepatch
+xen_action_hooks.livepatch
+xen_action_hooks_marker.livepatch
+xen_action_hooks_noapply.livepatch
+xen_action_hooks_norevert.livepatch);
my $livepatch_dir="/usr/lib/debug/livepatch";
my $xen_extra_info;
@@ -111,6 +118,25 @@ my @livepatch_tests = (
{ C => "xen-livepatch unload xen_nop", rc => 256 },
{ C => "xen-livepatch revert xen_nop" },
{ C => "xen-livepatch unload xen_nop" },
+{ C => "xen-livepatch load xen_no_xen_buildid.livepatch", rc => 256 },
+{ C => "xen-livepatch load xen_prepost_hooks_fail.livepatch", rc => 256 },
+{ C => "xen-livepatch load xen_prepost_hooks.livepatch" },
+# First revert fails
+{ C => "xen-livepatch revert xen_prepost_hooks", rc => 256 },
+{ C => "xen-livepatch revert xen_prepost_hooks" },
+{ C => "xen-livepatch unload xen_prepost_hooks" },
+{ C => "xen-livepatch load xen_action_hooks.livepatch" },
+{ C => "xen-livepatch revert xen_action_hooks" },
+{ C => "xen-livepatch unload xen_action_hooks" },
+{ C => "xen-livepatch load xen_action_hooks_marker.livepatch" },
+{ C => "xen-livepatch revert xen_action_hooks_marker" },
+{ C => "xen-livepatch unload xen_action_hooks_marker" },
+{ C => "xen-livepatch load xen_action_hooks_noapply.livepatch" },
+{ C => "xen-livepatch revert xen_action_hooks_noapply" },
+{ C => "xen-livepatch unload xen_action_hooks_noapply" },
+{ C => "xen-livepatch load xen_action_hooks_norevert.livepatch" },
+{ C => "xen-livepatch revert xen_action_hooks_norevert" },
+{ C => "xen-livepatch unload xen_action_hooks_norevert" },
);
# Copied from
https://stackoverflow.com/questions/11514947/capture-the-output-of-perl-system
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 08/12] livepatch: Add support for inline asm hotpatching expectations
> Ah, I forgot about endianness of course... > I am sending an improved patch. I hope it works this time as expected. Works nicely! (Tested only on ARM64, hadn't tried ARM32 yet). ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/12] livepatch: Add metadata runtime retrieval mechanism
On Tue, Aug 27, 2019 at 08:46:23AM +, Pawel Wieczorkiewicz wrote: > Extend the livepatch list operation to fetch also payloads' metadata. > This is achieved by extending the sysctl list interface with 2 extra > guest handles: > * metadata - an array of arbitrary size strings > * metadata_len - an array of metadata strings' lengths (uin32_t each) > > Payloads' metadata is a string of arbitrary size and does not have an > upper bound limit. It may also vary in size between payloads. > > In order to let the userland allocate enough space for the incoming > data add a metadata total size field to the list sysctl operation and > fill it with total size of all payloads' metadata. > > Extend the libxc to handle the metadata back-to-back data transfers > as well as metadata length array data transfers. > > The xen-livepatch userland tool is extended to always display the > metadata for each received module. The metadata is received with the > following format: key=value\0key=value\0...key=value\0. The format is > modified to the following one: key=value;key=value;...key=value. > The new format allows to easily parse the metadata for a given module > by a machine. > > Signed-off-by: Pawel Wieczorkiewicz > Reviewed-by: Andra-Irina Paraschiv > Reviewed-by: Martin Pohlack > Reviewed-by: Norbert Manthey > --- > Changed since v1: > * added corresponding documentation > * make metadata optional (do not display it when given payload > does not have it) > > docs/misc/livepatch.pandoc| 31 - > tools/libxc/include/xenctrl.h | 22 +++ > tools/libxc/xc_misc.c | 64 > +++ > tools/misc/xen-livepatch.c| 43 +++-- > xen/common/livepatch.c| 22 +++ > xen/include/public/sysctl.h | 19 + > 6 files changed, 157 insertions(+), 44 deletions(-) And it may be worth folding this in: diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile index a7857d3a2e..3f088e74b2 100644 --- a/xen/test/livepatch/Makefile +++ b/xen/test/livepatch/Makefile @@ -79,9 +79,17 @@ config.h: xen_hello_world_func.o xen_hello_world.o: config.h .PHONY: $(LIVEPATCH) -$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o xen_note.o +$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o xen_note.o modinfo.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^ +.PHONY: modinfo.o +modinfo.o: + (set -e; \ +printf "LIVEPATCH_RULEZ\0") > $@.bin + $(OBJCOPY) $(OBJCOPY_MAGIC) \ + --rename-section=.data=.modinfo,alloc,load,readonly,data,contents -S $@.bin $@ + #rm -f $@.bin + # # This target is only accessible if CONFIG_LIVEPATCH is defined, which # depends on $(build_id_linker) being available. Hence we do not .. which the test-case can test for. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Upstream Dom0 DRM problems regarding swiotlb
On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> >>> On 13.02.19 at 17:00, wrote:
> > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich wrote:
> >> >>> On 13.02.19 at 15:10, wrote:
> >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> >> > guest? That hadn't crossed my mind. Is there an easy way to find out
> >> > if we're a pv guest in the need_swiotlb conditionals?
> >>
> >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> >> the point if the check is already to be Xen-specific. There's no generic
> >> "is PV" predicate that I'm aware of.
> >
> > Well, that makes doing conditional code right more difficult. I
> > assume since there isn't a generic predicate, and PV isn't new, that
> > it's absence is by design? To reign in the temptation to sprinkle
> > conditional code all over the kernel? ;-)
>
> Well, with lguest gone, Xen is the only PV environment the kernel
> can run in, afaik at least. I guess to decide between the suggested
> options or the need for some abstracting macro (or yet something
> else), you'll really need to ask the driver maintainers. Or simply
> send a patch their way implementing one of them, and see what
> their reaction is.
Could you try this out and see if it works and I will send it out:
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 9fc3296592fe..96bf1df0ed28 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include "amdgpu.h"
#include "gmc_v6_0.h"
#include "amdgpu_ucode.h"
@@ -887,6 +888,8 @@ static int gmc_v6_0_sw_init(void *handle)
dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
}
adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+ if (xen_pv_domain())
+ adev->need_swiotlb = 1;
r = gmc_v6_0_init_microcode(adev);
if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 761dcfb2fec0..710ac0ece1b0 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include "amdgpu.h"
#include "cikd.h"
#include "cik.h"
@@ -1031,6 +1032,8 @@ static int gmc_v7_0_sw_init(void *handle)
pr_warn("amdgpu: No coherent DMA available\n");
}
adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+ if (xen_pv_domain())
+ adev->need_swiotlb = 1;
r = gmc_v7_0_init_microcode(adev);
if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 1ad7e6b8ed1d..c418a129bb32 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include "amdgpu.h"
#include "gmc_v8_0.h"
#include "amdgpu_ucode.h"
@@ -1156,6 +1157,8 @@ static int gmc_v8_0_sw_init(void *handle)
pr_warn("amdgpu: No coherent DMA available\n");
}
adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+ if (xen_pv_domain())
+ adev->need_swiotlb = 1;
r = gmc_v8_0_init_microcode(adev);
if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index bacdaef77b6c..85c0762c37ae 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -22,6 +22,7 @@
*/
#include
#include
+#include
#include "amdgpu.h"
#include "gmc_v9_0.h"
#include "amdgpu_atomfirmware.h"
@@ -1004,6 +1005,8 @@ static int gmc_v9_0_sw_init(void *handle)
printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
}
adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+ if (xen_pv_domain())
+ adev->need_swiotlb = 1;
if (adev->gmc.xgmi.supported) {
r = gfxhub_v1_1_get_xgmi_info(adev);
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index 59c8a6647ff2..02fba6829936 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include "radeon_reg.h"
#include "radeon.h"
#include "atom.h"
@@ -1388,6 +1389,8 @@ int radeon_device_init(struct radeon_device *rdev,
pr_warn("radeon: No coherent DMA available\n");
}
rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+ if (xen_pv_domain())
+ rdev->need_swiotlb = 1;
/* Registers mapping */
/* TODO: block userspace mapping of io register */
>
> Jan
>
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenpro
Re: [Xen-devel] Upstream Dom0 DRM problems regarding swiotlb
On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
> wrote:
> >
> > On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
> > wrote:
> > >
> > > On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> > > > >>> On 13.02.19 at 17:00, wrote:
> > > > > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich wrote:
> > > > >> >>> On 13.02.19 at 15:10, wrote:
> > > > >> > Ah, so this isn't necessarily Xen-specific but rather any
> > > > >> > paravirtual
> > > > >> > guest? That hadn't crossed my mind. Is there an easy way to find
> > > > >> > out
> > > > >> > if we're a pv guest in the need_swiotlb conditionals?
> > > > >>
> > > > >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> > > > >> the point if the check is already to be Xen-specific. There's no
> > > > >> generic
> > > > >> "is PV" predicate that I'm aware of.
> > > > >
> > > > > Well, that makes doing conditional code right more difficult. I
> > > > > assume since there isn't a generic predicate, and PV isn't new, that
> > > > > it's absence is by design? To reign in the temptation to sprinkle
> > > > > conditional code all over the kernel? ;-)
> > > >
> > > > Well, with lguest gone, Xen is the only PV environment the kernel
> > > > can run in, afaik at least. I guess to decide between the suggested
> > > > options or the need for some abstracting macro (or yet something
> > > > else), you'll really need to ask the driver maintainers. Or simply
> > > > send a patch their way implementing one of them, and see what
> > > > their reaction is.
> > >
> > > Could you try this out and see if it works and I will send it out:
> > >
> *snip*
> >
> > Yes, that works for me. However, I feel like the conditional should
> > be in drm_get_max_iomem() instead of directly after it everywhere it's
> > used... and is just checking xen_pv_domain() enough? Jan made it
> > sound like there were possibly other PV cases that would also still
> > need swiotlb.
>
> How about this? It strcmp's pv_info to see if we're bare metal, does
> the comparison in a single place, moves the bit shifting comparison
> into the function (simplifying the drm driver code), and renames the
> function to more aptly describe what's going on.
That looks much better.
Would love to see this posted upstream!
>
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> index 73ad02aea2b2..328d45b8b2ec 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> @@ -885,7 +885,7 @@ static int gmc_v6_0_sw_init(void *handle)
> pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
> }
> -adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
>
> r = gmc_v6_0_init_microcode(adev);
> if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index 910c4ce19cb3..3d49eff28448 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -1029,7 +1029,7 @@ static int gmc_v7_0_sw_init(void *handle)
> pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> pr_warn("amdgpu: No coherent DMA available\n");
> }
> -adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
>
> r = gmc_v7_0_init_microcode(adev);
> if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 747c068379dc..9247dd6316f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -1155,7 +1155,7 @@ static int gmc_v8_0_sw_init(void *handle)
> pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> pr_warn("amdgpu: No coherent DMA available\n");
> }
> -adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +adev->need_swiotlb =
Re: [Xen-devel] Upstream Dom0 DRM problems regarding swiotlb
On Fri, Feb 15, 2019 at 11:07:22AM -0500, Michael Labriola wrote:
> On Fri, Feb 15, 2019 at 12:57 AM Juergen Gross wrote:
> >
> > On 14/02/2019 18:57, Christoph Hellwig wrote:
> > > On Thu, Feb 14, 2019 at 07:03:38AM +0100, Juergen Gross wrote:
> > >>> The thing which is different between Xen PV guests and most others (all
> > >>> others(?), now that Lguest and UML have been dropped) is that what Linux
> > >>> thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
> > >>> physical address space.
> > >>>
> > >>> Therefore, code which has a buffer spanning a page boundary can't just
> > >>> convert a pointer to the buffer into a physical address, and hand that
> > >>> address to a device. You generally end up with either memory corruption
> > >>> (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
> > >>> (DMA hitting a pages which isn't allocated to the guest).
> > >
> > > The Linux DMA API allows for dma_map_page / dma_map_single calls to
> > > spawn 4k boundaries. If Xen doesn't support that it will have to bounce
> > > buffer for that case (and get horrible performance).
> > >
> > > But the latter text seems to agree with that. So what is the actual
> > > problem that started this discussion?
> > >
> >
> > See https://lists.xen.org/archives/html/xen-devel/2019-02/threads.html#00818
>
> I believe the actual problem is either:
>
> 1) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *should* work on a Xen PV host, but doesn't and needs to be
> fixed.
>
> or
>
> 2) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *cannot* work in Xen, and they should go back to calling
> ttm_dma_populate() in that case.
The Nvidia one has this (correct):
1583 #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)
1584 if (swiotlb_nr_tbl()) {
1585 return ttm_dma_populate((void *)ttm, dev, ctx);
1586 }
1587 #endif
The Radeon has this - where now it adds 'need_swiotlb':
695 #ifdef CONFIG_SWIOTLB
696 if (rdev->need_swiotlb && swiotlb_nr_tbl()) {
697 return ttm_dma_populate(>t->ttm, rdev->dev, ctx);
698 }
699 #endif
The problem is fairly simple - the platform _requires_ to use
DMA API.
But the driver's have their own 'need_swiotlb' which ignores the platform
and sets it based on the device's DMA width:
rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
There should be an extra check to see if the platform requires
to use DMA API.
>
> I'm having a hard time figuring out which of those is correct.
>
> --
> Michael D Labriola
> 21 Rip Van Winkle Cir
> Warwick, RI 02886
> 401-316-9844 (cell)
> 401-848-8871 (work)
> 401-234-1306 (home)
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/pciback: Don't disable PCI_COMMAND on PCI device reset.
On Wed, Feb 13, 2019 at 06:21:31PM -0500, Prarit Bhargava wrote:
> From: Konrad Rzeszutek Wilk
>
+LKML
> This was submitted in 2015 here
>
> https://marc.info/?l=linux-kernel&m=142807132515973&w=2
>
> and has been included in Fedora builds ever since. No issues have been
> reported with the patch.
>
> P.
>
> 8<
>
> There is no need for this at all. Worst it means that if
> the guest tries to write to BARs it could lead (on certain
> platforms) to PCI SERR errors.
>
> Please note that with af6fc858a35b90e89ea7a7ee58e66628c55c776b
> "xen-pciback: limit guest control of command register"
> a guest is still allowed to enable those control bits (safely), but
> is not allowed to disable them and that therefore a well behaved
> frontend which enables things before using them will still
> function correctly.
>
> This is done via an write to the configuration register 0x4 which
> triggers on the backend side:
> command_write
> \- pci_enable_device
> \- pci_enable_device_flags
> \- do_pci_enable_device
>\- pcibios_enable_device
> \-pci_enable_resourcess
> [which enables the PCI_COMMAND_MEMORY|PCI_COMMAND_IO]
>
> However guests (and drivers) which don't do this could cause
> problems, including the security issues which XSA-120 sought
> to address.
>
> Reported-by: Jan Beulich
> Signed-off-by: Konrad Rzeszutek Wilk
> Reviewed-by: Prarit Bhargava
> Cc: Juergen Gross
> ---
> drivers/xen/xen-pciback/pciback_ops.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/xen/xen-pciback/pciback_ops.c
> b/drivers/xen/xen-pciback/pciback_ops.c
> index ea4a08b83fa0..787966f44589 100644
> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -127,8 +127,6 @@ void xen_pcibk_reset_device(struct pci_dev *dev)
> if (pci_is_enabled(dev))
> pci_disable_device(dev);
>
> - pci_write_config_word(dev, PCI_COMMAND, 0);
> -
> dev->is_busmaster = 0;
> } else {
> pci_read_config_word(dev, PCI_COMMAND, &cmd);
> --
> 2.18.1
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC 20/39] xen-blkback: module_exit support
On Wed, Feb 20, 2019 at 08:15:50PM +, Joao Martins wrote:
>
> Implement module_exit to allow users to do module unload of blkback.
> We prevent users from module unload whenever there are still interfaces
> allocated, in other words, do module_get on xen_blkif_alloc() and
> module_put on xen_blkif_free().
This patch looks like it can go now in right?
>
> Signed-off-by: Joao Martins
> ---
> drivers/block/xen-blkback/blkback.c | 8
> drivers/block/xen-blkback/common.h | 2 ++
> drivers/block/xen-blkback/xenbus.c | 14 ++
> 3 files changed, 24 insertions(+)
>
> diff --git a/drivers/block/xen-blkback/blkback.c
> b/drivers/block/xen-blkback/blkback.c
> index fd1e19f1a49f..d51d88be88e1 100644
> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -1504,5 +1504,13 @@ static int __init xen_blkif_init(void)
>
> module_init(xen_blkif_init);
>
> +static void __exit xen_blkif_exit(void)
> +{
> + xen_blkif_interface_exit();
> + xen_blkif_xenbus_exit();
> +}
> +
> +module_exit(xen_blkif_exit);
> +
> MODULE_LICENSE("Dual BSD/GPL");
> MODULE_ALIAS("xen-backend:vbd");
> diff --git a/drivers/block/xen-blkback/common.h
> b/drivers/block/xen-blkback/common.h
> index 1d3002d773f7..3415c558e115 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -376,8 +376,10 @@ struct phys_req {
> blkif_sector_t sector_number;
> };
> int xen_blkif_interface_init(void);
> +void xen_blkif_interface_exit(void);
>
> int xen_blkif_xenbus_init(void);
> +void xen_blkif_xenbus_exit(void);
>
> irqreturn_t xen_blkif_be_int(int irq, void *dev_id);
> int xen_blkif_schedule(void *arg);
> diff --git a/drivers/block/xen-blkback/xenbus.c
> b/drivers/block/xen-blkback/xenbus.c
> index a4bc74e72c39..424e2efebe85 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -181,6 +181,8 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
> init_completion(&blkif->drain_complete);
> INIT_WORK(&blkif->free_work, xen_blkif_deferred_free);
>
> + __module_get(THIS_MODULE);
> +
> return blkif;
> }
>
> @@ -328,6 +330,8 @@ static void xen_blkif_free(struct xen_blkif *blkif)
>
> /* Make sure everything is drained before shutting down */
> kmem_cache_free(xen_blkif_cachep, blkif);
> +
> + module_put(THIS_MODULE);
> }
>
> int __init xen_blkif_interface_init(void)
> @@ -341,6 +345,11 @@ int __init xen_blkif_interface_init(void)
> return 0;
> }
>
> +void xen_blkif_interface_exit(void)
> +{
> + kmem_cache_destroy(xen_blkif_cachep);
> +}
> +
> /*
> * sysfs interface for VBD I/O requests
> */
> @@ -1115,3 +1124,8 @@ int xen_blkif_xenbus_init(void)
> {
> return xenbus_register_backend(&xen_blkbk_driver);
> }
> +
> +void xen_blkif_xenbus_exit(void)
> +{
> + xenbus_unregister_driver(&xen_blkbk_driver);
> +}
> --
> 2.11.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] drm: add func to better detect wether swiotlb is needed
.snip..
> > -u64 drm_get_max_iomem(void)
> > +bool drm_need_swiotlb(int dma_bits)
> > {
> > struct resource *tmp;
> > resource_size_t max_iomem = 0;
> >
> > + /*
> > +* Xen paravirtual hosts require swiotlb regardless of requested dma
> > +* transfer size.
> > +*
> > +* NOTE: Really, what it requires is use of the dma_alloc_coherent
> > +* allocator used in ttm_dma_populate() instead of
> > +* ttm_populate_and_map_pages(), which bounce buffers so much
> > in
> > +* Xen it leads to swiotlb buffer exhaustion.
> > +*/
> > + if (xen_pv_domain())
>
> I've not been following all of the ins and outs of the discussion on this so
> apologies if I'm missing some context, but...
>
> This looks like the wrong test to me. I think it should be:
>
> if ( xen_swiotlb )
Ah, that could be as well.
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] Revert "swiotlb: remove SWIOTLB_MAP_ERROR"
On Mon, Mar 04, 2019 at 08:59:03PM +0100, Arnd Bergmann wrote:
> This reverts commit b907e20508d0 ("swiotlb: remove SWIOTLB_MAP_ERROR"), which
> introduced an overflow warning in configurations that have a larger
> dma_addr_t than phys_addr_t:
>
> In file included from include/linux/dma-direct.h:5,
> from kernel/dma/swiotlb.c:23:
> kernel/dma/swiotlb.c: In function 'swiotlb_tbl_map_single':
> include/linux/dma-mapping.h:136:28: error: conversion from 'long long
> unsigned int' to 'phys_addr_t' {aka 'unsigned int'} changes value from
> '18446744073709551615' to '4294967295' [-Werror=overflow]
> #define DMA_MAPPING_ERROR (~(dma_addr_t)0)
> ^
> kernel/dma/swiotlb.c:544:9: note: in expansion of macro 'DMA_MAPPING_ERROR'
> return DMA_MAPPING_ERROR;
>
> The configuration that caused this is on 32-bit ARM, where the DMA address
> space depends on the enabled hardware platforms, while the physical
> address space depends on the type of MMU chosen (classic vs LPAE).
>
> I tried a couple of alternative approaches, but the previous version
> seems as good as any other, so I went back to that.
That is really a bummer.
What about making the phys_addr_t and dma_addr_t have the same
width with some magic #ifdef hackery?
>
> Fixes: b907e20508d0 ("swiotlb: remove SWIOTLB_MAP_ERROR")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/xen/swiotlb-xen.c | 4 ++--
> include/linux/swiotlb.h | 3 +++
> kernel/dma/swiotlb.c | 4 ++--
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 877baf2a94f4..57a98279bf4f 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -406,7 +406,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
>
> map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
>attrs);
> - if (map == DMA_MAPPING_ERROR)
> + if (map == SWIOTLB_MAP_ERROR)
> return DMA_MAPPING_ERROR;
>
> dev_addr = xen_phys_to_bus(map);
> @@ -557,7 +557,7 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct
> scatterlist *sgl,
>sg_phys(sg),
>sg->length,
>dir, attrs);
> - if (map == DMA_MAPPING_ERROR) {
> + if (map == SWIOTLB_MAP_ERROR) {
> dev_warn(hwdev, "swiotlb buffer is full\n");
> /* Don't panic here, we expect map_sg users
> to do proper error handling. */
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 361f62bb4a8e..a65a36551f58 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -44,6 +44,9 @@ enum dma_sync_target {
> SYNC_FOR_DEVICE = 1,
> };
>
> +/* define the last possible byte of physical address space as a mapping
> error */
> +#define SWIOTLB_MAP_ERROR (~(phys_addr_t)0x0)
> +
> extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dma_addr_t tbl_dma_addr,
> phys_addr_t phys, size_t size,
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 12059b78b631..922880b84387 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -541,7 +541,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> spin_unlock_irqrestore(&io_tlb_lock, flags);
> if (!(attrs & DMA_ATTR_NO_WARN) && printk_ratelimit())
> dev_warn(hwdev, "swiotlb buffer is full (sz: %zd bytes)\n",
> size);
> - return DMA_MAPPING_ERROR;
> + return SWIOTLB_MAP_ERROR;
> found:
> io_tlb_used += nslots;
> spin_unlock_irqrestore(&io_tlb_lock, flags);
> @@ -659,7 +659,7 @@ bool swiotlb_map(struct device *dev, phys_addr_t *phys,
> dma_addr_t *dma_addr,
> /* Oh well, have to allocate and map a bounce buffer. */
> *phys = swiotlb_tbl_map_single(dev, __phys_to_dma(dev, io_tlb_start),
> *phys, size, dir, attrs);
> - if (*phys == DMA_MAPPING_ERROR)
> + if (*phys == SWIOTLB_MAP_ERROR)
> return false;
>
> /* Ensure that the address returned is DMA'ble */
> --
> 2.20.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [GIT PULL] (xen) stable/for-jens-5.1 to your 'for-5.1/block' branch.
Hi Jens, Apologies for doing it right at the merge window time. This patchset has been brewing for quite a while. git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git stable/for-jens-5.1 This patchset makes the backend more robust by reading a negotiation variable only once and not twice. Thank you! drivers/block/xen-blkback/xenbus.c | 99 ++ 1 file changed, 57 insertions(+), 42 deletions(-) Dongli Zhang (2): xen/blkback: add stack variable 'blkif' in connect_ring() xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront signature.asc Description: PGP signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] MAINTAINERS: add myself as maintainer for public I/O interfaces
On Tue, Mar 12, 2019 at 02:24:14PM +0100, Juergen Gross wrote: > The "PUBLIC I/O INTERFACES AND PV DRIVERS DESIGNS" section of the > MAINTAINERS file lists Konrad as the only maintainer. Add myself for > helping him to review patches. > > Signed-off-by: Juergen Gross Reviewed-by: Konrad Rzeszutek Wilk Thank you! > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index a0cda4f7a1..ba7527c423 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -329,6 +329,7 @@ F:xen/include/acpi/cpufreq/ > > PUBLIC I/O INTERFACES AND PV DRIVERS DESIGNS > M: Konrad Rzeszutek Wilk > +M: Juergen Gross > S: Supported > F: xen/include/public/io/ > > -- > 2.16.4 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on "sectors" self-consistent
On Wed, Mar 20, 2019 at 12:52:28PM +, Paul Durrant wrote:
> Currently the comment at line #267 claims that the value should be
> expressed in number logical sectors, whereas the comment at line #613
> states that the value should be expressed strictly in units of 512 bytes.
>
> Signed-off-by: Paul Durrant
> ---
> Cc: Konrad Rzeszutek Wilk
>
> Looking at xen-blkfront in Linux, I'm also not convinced that it would
> function correctly is sector-size != 512 anyway so I wonder whether this
> patch should go further and define that sector-size is strictly 512.
I thought it actually did work with a CD-ROM backed ISO using blkfront?
> ---
> xen/include/public/io/blkif.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 15a71e3fea..d7c904d9dc 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -264,8 +264,7 @@
> * sectors
> * Values:
> *
> - * The size of the backend device, expressed in units of its logical
> - * sector size ("sector-size").
> + * The size of the backend device, expressed in units of 512 bytes.
> *
>
> *
> *Frontend XenBus Nodes
> --
> 2.20.1.2.gb21ebb671
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] public/io/blkif.h: make the comments on "sectors" self-consistent
On Wed, Mar 20, 2019 at 02:05:15PM +, Paul Durrant wrote:
> > -Original Message-
> > From: Paul Durrant
> > Sent: 20 March 2019 13:59
> > To: 'Konrad Rzeszutek Wilk'
> > Cc: xen-devel@lists.xenproject.org
> > Subject: RE: [PATCH] public/io/blkif.h: make the comments on "sectors"
> > self-consistent
> >
> > > -Original Message-
> > > From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com]
> > > Sent: 20 March 2019 13:53
> > > To: Paul Durrant
> > > Cc: xen-devel@lists.xenproject.org
> > > Subject: Re: [PATCH] public/io/blkif.h: make the comments on "sectors"
> > > self-consistent
> > >
> > > On Wed, Mar 20, 2019 at 12:52:28PM +, Paul Durrant wrote:
> > > > Currently the comment at line #267 claims that the value should be
> > > > expressed in number logical sectors, whereas the comment at line #613
> > > > states that the value should be expressed strictly in units of 512
> > > > bytes.
> > > >
> > > > Signed-off-by: Paul Durrant
> > > > ---
> > > > Cc: Konrad Rzeszutek Wilk
> > > >
> > > > Looking at xen-blkfront in Linux, I'm also not convinced that it would
> > > > function correctly is sector-size != 512 anyway so I wonder whether this
> > > > patch should go further and define that sector-size is strictly 512.
> > >
> > > I thought it actually did work with a CD-ROM backed ISO using blkfront?
> > >
> >
> > I've not tried that. What worries me is the call to xlvbd_alloc_gendisk()
> > which takes sectors as an
> > argument and passes it through to set_capacity() without scaling with
> > sector-size in any way, which is
> > would presumably need to do is sector-size != 512 (if we believe that
> > sectors should be in terms of
> > 512 byte units).
>
> Looking at xen-blkback, this actually just echoes the result of
> get_capacity() into 'sectors', which would suggest the comment at line #613
> is the one that is bogus... but how many other frontends have been coded
> against that? So, it would seem to me that the only way to get out of this
> compatibility mess is to make sector-size strictly 512.
Bye bye 4096 sectore-size :-)
Ugh, and we do <<9 all over the code so it is fairly baked.
Reviewed-by: Konrad Rzeszutek Wilk
>
> Paul
>
> >
> > Paul
> >
> > > > ---
> > > > xen/include/public/io/blkif.h | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/xen/include/public/io/blkif.h
> > > > b/xen/include/public/io/blkif.h
> > > > index 15a71e3fea..d7c904d9dc 100644
> > > > --- a/xen/include/public/io/blkif.h
> > > > +++ b/xen/include/public/io/blkif.h
> > > > @@ -264,8 +264,7 @@
> > > > * sectors
> > > > * Values:
> > > > *
> > > > - * The size of the backend device, expressed in units of its
> > > > logical
> > > > - * sector size ("sector-size").
> > > > + * The size of the backend device, expressed in units of 512
> > > > bytes.
> > > > *
> > > >
> > > > *
> > > > *Frontend XenBus Nodes
> > > > --
> > > > 2.20.1.2.gb21ebb671
> > > >
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/livepatch: Fail the build if duplicate symbols exist
On Fri, Apr 05, 2019 at 08:26:04PM +0100, Andrew Cooper wrote:
> From: Ross Lagerwall
>
> The binary diffing algorithm used by xen-livepatch depends on having unique
> symbols.
>
> Signed-off-by: Ross Lagerwall
> Signed-off-by: Andrew Cooper
Reviewed-by: Konrad Rzeszutek Wilk
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Ross Lagerwall
> CC: Konrad Rzeszutek Wilk
> CC: Norbert Manthey
>
> This patch has been part of the XenServer patchqueue for ages, and should have
> been upstreamed sooner. It obviously can't be applied while the "block
> speculation" issue is outstanding.
> ---
> xen/arch/x86/Makefile | 1 +
> xen/tools/symbols.c | 11 +--
> 2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index ef09939..d4ae46b 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -98,6 +98,7 @@ endif
>
> syms-warn-dup-y := --warn-dup
> syms-warn-dup-$(CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS) :=
> +syms-warn-dup-$(CONFIG_LIVEPATCH) := --error-dup
>
> $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET)
> $(XEN_IMG_OFFSET) \
> diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
> index 05139d1..9f9e2c9 100644
> --- a/xen/tools/symbols.c
> +++ b/xen/tools/symbols.c
> @@ -599,7 +599,7 @@ static int compare_name(const void *p1, const void *p2)
> int main(int argc, char **argv)
> {
> unsigned int i;
> - bool unsorted = false, warn_dup = false;
> + bool unsorted = false, warn_dup = false, error_dup = false, found_dup =
> false;
>
> if (argc >= 2) {
> for (i = 1; i < argc; i++) {
> @@ -619,6 +619,8 @@ int main(int argc, char **argv)
> sort_by_name = 1;
> else if (strcmp(argv[i], "--warn-dup") == 0)
> warn_dup = true;
> + else if (strcmp(argv[i], "--error-dup") == 0)
> + warn_dup = error_dup = true;
> else if (strcmp(argv[i], "--xensyms") == 0)
> map_only = true;
> else
> @@ -634,14 +636,19 @@ int main(int argc, char **argv)
> for (i = 1; i < table_cnt; ++i)
> if (strcmp(SYMBOL_NAME(table + i - 1),
> SYMBOL_NAME(table + i)) == 0 &&
> - table[i - 1].addr != table[i].addr)
> + table[i - 1].addr != table[i].addr) {
> fprintf(stderr,
> "Duplicate symbol '%s' (%llx !=
> %llx)\n",
> SYMBOL_NAME(table + i),
> table[i].addr, table[i - 1].addr);
> + found_dup = true;
> + }
> unsorted = true;
> }
>
> + if (error_dup && found_dup)
> + exit(1);
> +
> if (unsorted)
> qsort(table, table_cnt, sizeof(*table), compare_value);
>
> --
> 2.1.4
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] livepatch-build-tools: fix section mismatch
On Fri, Apr 12, 2019 at 01:21:48PM +1200, Glenn Enright wrote: > HI all > > A quick bump on this, and added Andrew to the CC list since he appears to > have looked at livepatch 'stuff' recently. > > Was this patch of any interest at all? Or just wrong? After reading patch > submission guidlines, should I resend this a different way? What patch? Please send it to xen-devel mailing list and use git-send-email. And also CC the maintainers -which are me and Ross. Thanks. > > Regards, Glenn > > On 4/1/19 3:28 PM, Glenn Enright wrote: > > I hit an issue generating a livepatch for a recent xsa. I saw the > > following lines from the create-diff-object.log ... > > > > /livepatch-build-tools/create-diff-object: ERROR: grant_table.o: > > kpatch_regenerate_special_section: 1162: group size mismatch for section > > .altinstructions > > > > This looks really similar to the issue reported and fixed in > > https://github.com/dynup/kpatch/pull/528 > > > > The attached patch is based on that report, and resulted in a good > > livepatch. The alt section size in my case was actually 14. > > > > Regards, Glenn > > > > ___ > > Xen-devel mailing list > > Xen-devel@lists.xenproject.org > > https://lists.xenproject.org/mailman/listinfo/xen-devel > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch: independ. modules 1/3] livepatch: Always check hypervisor build ID upon hotpatch upload
On Tue, Apr 16, 2019 at 12:58:30PM +, Pawel Wieczorkiewicz wrote:
> This change is part of a independant stacked hotpatch modules
> feature. This feature allows to bypass dependencies between modules
> upon loading, but still verifies Xen build ID matching.
OK, so build_id_dep would not be called for those?
I see patch #2 doing this. Cool.
>
> In order to prevent (up)loading any hotpatches built for different
> hypervisor version as indicated by the Xen Build ID, add checking for
> the payload's vs Xen's build id match.
>
> To achieve that embed into every hotpatch another section with a
> dedicated hypervisor build id in it. After the payload is loaded and
> the .livepatch.xen_depends section becomes available, perform the
> check and reject the payload if there is no match.
>
> Signed-off-by: Pawel Wieczorkiewicz
> Reviewed-by: Andra-Irina Paraschiv
> Reviewed-by: Bjoern Doebel
> Reviewed-by: Eslam Elnikety
> Reviewed-by: Martin Pohlack
> ---
> xen/common/livepatch.c | 47
> +
> xen/include/xen/livepatch.h | 7 ---
Can you please include:
- Changes to the docs/misc/livepatch.markdown describing this change.
- Include a test-case exercising this.
- Fix the test-cases as I think they will now all fail? (As they don't have
this section?)
Thank you.
> 2 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index d6eaae6d3b..6a4af6ce57 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -74,6 +74,7 @@ struct payload {
> unsigned int nsyms; /* Nr of entries in .strtab and
> symbols. */
> struct livepatch_build_id id;/* ELFNOTE_DESC(.note.gnu.build-id)
> of the payload. */
> struct livepatch_build_id dep; /*
> ELFNOTE_DESC(.livepatch.depends). */
> +struct livepatch_build_id xen_dep; /*
> ELFNOTE_DESC(.livepatch.xen_depends). */
> livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call
> after */
> livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the
> payload. */
> unsigned int n_load_funcs; /* Nr of the funcs to load and
> execute. */
> @@ -476,11 +477,34 @@ static bool section_ok(const struct livepatch_elf *elf,
> return true;
> }
>
> +static int check_xen_build_id(const struct payload *payload)
> +{
> +const void *id = NULL;
> +unsigned int len = 0;
> +int rc;
> +
> +ASSERT(payload->xen_dep.len);
> +ASSERT(payload->xen_dep.p);
> +
> +rc = xen_build_id(&id, &len);
> +if ( rc )
> +return rc;
> +
> +if ( payload->xen_dep.len != len || memcmp(id, payload->xen_dep.p, len)
> ) {
> +dprintk(XENLOG_ERR, "%s%s: check against hypervisor build-id
> failed!\n",
> +LIVEPATCH, payload->name);
> +return -EINVAL;
> +}
> +
> +return 0;
> +}
> +
> static int check_special_sections(const struct livepatch_elf *elf)
> {
> unsigned int i;
> static const char *const names[] = { ELF_LIVEPATCH_FUNC,
> ELF_LIVEPATCH_DEPENDS,
> + ELF_LIVEPATCH_XEN_DEPENDS,
> ELF_BUILD_ID_NOTE};
> DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
>
> @@ -632,6 +656,22 @@ static int prepare_payload(struct payload *payload,
> return -EINVAL;
> }
>
> +sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
> +if ( sec )
> +{
> +n = sec->load_addr;
> +
> +if ( sec->sec->sh_size <= sizeof(*n) )
> +return -EINVAL;
> +
> +if ( xen_build_id_check(n, sec->sec->sh_size,
> +&payload->xen_dep.p, &payload->xen_dep.len) )
> +return -EINVAL;
> +
> +if ( !payload->xen_dep.len || !payload->xen_dep.p )
> +return -EINVAL;
> +}
> +
> /* Setup the virtual region with proper data. */
> region = &payload->region;
>
> @@ -882,6 +922,10 @@ static int load_payload_data(struct payload *payload,
> void *raw, size_t len)
> if ( rc )
> goto out;
>
> +rc = check_xen_build_id(payload);
> +if ( rc )
> +goto out;
> +
> rc = build_symbol_table(payload, &elf);
> if ( rc )
> goto out;
> @@ -1655,6 +1699,9 @@ static void livepatch_printall(unsigned char key)
>
> if ( data->dep.len )
> printk("depend-on=%*phN\n", data->dep.len, data->dep.p);
> +
> +if ( data->xen_dep.len )
> +printk("depend-on-xen=%*phN\n", data->xen_dep.len,
> data->xen_dep.p);
> }
>
> spin_unlock(&payload_lock);
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 1b1817ca0d..ed997aa4cc 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -29,9 +29,10 @@ struct xen_sysctl_livepatch_op;
> /* Convenienc
Re: [Xen-devel] [PATCH] livepatch-build-tools: Detect special section group sizes
On Tue, Apr 16, 2019 at 11:05:49AM +0100, Ross Lagerwall wrote: > On 4/12/19 5:50 AM, Glenn Enright wrote: > > A recent xsa livepatch failed to generate due to the following > > message in create-diff-object.log ... > > > > /livepatch-build-tools/create-diff-object: ERROR: grant_table.o: > > kpatch_regenerate_special_section: 1162: group size mismatch for section > > .altinstructions > > > > This is similar to the issue reported and fixed in > > https://github.com/dynup/kpatch/pull/528 which says ... > > "Hard-coding the special section group sizes is unreliable. > > Instead, determine them dynamically by finding the related > > struct definitions in the DWARF metadata." > > > > Signed-off-by: Glenn Enright > > --- > > CC: Ross Lagerwall > > CC: Konrad Rzeszutek Wilk > > > > This patch resulted in a loadable livepatch. The alt section size in my > > case was actually 14. > Thanks for the patch! > > Reviewed-by: Ross Lagerwall Could you resend it with the Reviewed-by please and include me on the To list please? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch-build-tools: independ. modules] livepatch-build: Embed hypervisor build id into every hotpatch
On Tue, Apr 16, 2019 at 12:57:14PM +, Pawel Wieczorkiewicz wrote:
> This change is part of a independant stacked hotpatch modules
> feature. This feature allows to bypass dependencies between modules
> upon loading, but still verifies Xen build ID matching.
>
> With stacked hotpatch modules it is essential that each and every
> hotpatch is verified against the hypervisor build id upon upload.
> It must not be possible to successfully upload hotpatches built for
> incorrect version of the hypervisor.
>
> To achieve that always embed an additional ELF section:
> '.livpatch.xen_depends' containing the hypervisor build id.
>
> The hypervisor build id must be always provided as a command line
> parameter: --xen-depends.
>
> Signed-off-by: Pawel Wieczorkiewicz
> Reviewed-by: Andra-Irina Paraschiv
> Reviewed-by: Bjoern Doebel
> Reviewed-by: Norbert Manthey
This patch looks OK, but I would want to wait until the Xen hypervisor
one gets the test-cases+documentation changes..
> ---
> livepatch-build | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/livepatch-build b/livepatch-build
> index c057fa1..0938b3a 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -30,6 +30,7 @@ DEBUG=n
> XEN_DEBUG=n
> SKIP=
> DEPENDS=
> +XEN_DEPENDS=
> PRELINK=
> XENSYMS=xen-syms
>
> @@ -157,6 +158,9 @@ function create_patch()
> # Create a dependency section
> perl -e "print pack 'VVVZ*H*', 4, 20, 3, 'GNU', '${DEPENDS}'" >
> depends.bin
>
> +# Create a Xen dependency section
> +perl -e "print pack 'VVVZ*H*', 4, 20, 3, 'GNU', '${XEN_DEPENDS}'" >
> xen_depends.bin
> +
> echo "Creating patch module..."
> if [ -z "$PRELINK" ]; then
> ld -r -o "${PATCHNAME}.livepatch" --build-id=sha1 $(find output
> -type f -name "*.o") || die
> @@ -168,6 +172,9 @@ function create_patch()
>
> objcopy --add-section .livepatch.depends=depends.bin
> "${PATCHNAME}.livepatch"
> objcopy --set-section-flags .livepatch.depends=alloc,readonly
> "${PATCHNAME}.livepatch"
> +
> +objcopy --add-section .livepatch.xen_depends=xen_depends.bin
> "${PATCHNAME}.livepatch"
> +objcopy --set-section-flags .livepatch.xen_depends=alloc,readonly
> "${PATCHNAME}.livepatch"
> }
>
> usage() {
> @@ -183,12 +190,13 @@ usage() {
> echo "--xen-debugBuild debug Xen (if your .config does
> not have the options)" >&2
> echo "--xen-syms Build against a xen-syms" >&2
> echo "--depends Required build-id" >&2
> +echo "--xen-depends Required Xen build-id" >&2
> echo "--prelink Prelink" >&2
> }
>
> find_tools || die "can't find supporting tools"
>
> -options=$(getopt -o hs:p:c:o:j:k:d -l
> "help,srcdir:,patch:,config:,output:,cpus:,skip:,debug,xen-debug,xen-syms:,depends:,prelink"
> -- "$@") || die "getopt failed"
> +options=$(getopt -o hs:p:c:o:j:k:d -l
> "help,srcdir:,patch:,config:,output:,cpus:,skip:,debug,xen-debug,xen-syms:,depends:,xen-depends:,prelink"
> -- "$@") || die "getopt failed"
>
> eval set -- "$options"
>
> @@ -247,6 +255,11 @@ while [[ $# -gt 0 ]]; do
> DEPENDS="$1"
> shift
> ;;
> +--xen-depends)
> +shift
> +XEN_DEPENDS="$1"
> +shift
> +;;
> --prelink)
> PRELINK=--resolve
> shift
> @@ -263,6 +276,7 @@ done
> [ -z "$configarg" ] && die ".config not given"
> [ -z "$outputarg" ] && die "Output directory not given"
> [ -z "$DEPENDS" ] && die "Build-id dependency not given"
> +[ -z "$XEN_DEPENDS" ] && die "Xen Build-id dependency not given"
>
> SRCDIR="$(readlink -m -- "$srcarg")"
> PATCHFILE="$(readlink -m -- "$patcharg")"
> --
> 2.16.5
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
> Ust-ID: DE 289 237 879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
>
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section
On Tue, Apr 16, 2019 at 12:22:39PM +, Pawel Wieczorkiewicz wrote:
> When there is no changed function in the generated payload, do not
> create an empty .livepatch.funcs section. Hypervisor code considers
> such payloads as broken and rejects to load them.
>
> Such payloads without any changed functions may appear when only
> hooks are specified.
Ross, I am going to push this in next week unless you have other thoughts?
>
> Signed-off-by: Pawel Wieczorkiewicz
> Reviewed-by: Martin Mazein
> Reviewed-by: Martin Pohlack
>
> CR: https://code.amazon.com/reviews/CR-7368634
> ---
> create-diff-object.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 82f777e..af2245c 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1744,6 +1744,11 @@ static void livepatch_create_patches_sections(struct
> kpatch_elf *kelf,
> if (sym->type == STT_FUNC && sym->status == CHANGED)
> nr++;
>
> + if (nr == 0) {
> + log_debug("No changed functions found. Skipping
> .livepatch.funcs section creation\n");
> + return;
> + }
> +
> /* create text/rela section pair */
> sec = create_section_pair(kelf, ".livepatch.funcs", sizeof(*funcs), nr);
> relasec = sec->rela;
> --
> 2.16.5
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
> Ust-ID: DE 289 237 879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
>
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes
On Tue, Apr 16, 2019 at 12:07:14PM +, Pawel Wieczorkiewicz wrote:
> Hard-coding the special section group sizes is unreliable. Instead,
> determine them dynamically by finding the related struct definitions
> in the DWARF metadata.
>
> This is a livepatch backport of kpatch upstream commit [1]:
> kpatch-build: detect special section group sizes 170449847136a48b19fc
>
> Xen only deals with alt_instr, bug_frame and exception_table_entry
> structures, so sizes of these structers are obtained from xen-syms.
>
> This change is needed since with recent Xen the alt_instr structure
> has changed size from 12 to 14 bytes.
Oh this is so much better than the "solution" we coded.
Thank you!
Ross, will commit to repo unless you have concerns..
>
> [1]
> https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56
>
> Signed-off-by: Pawel Wieczorkiewicz
> Reviewed-by: Bjoern Doebel
> Reviewed-by: Martin Mazein
> ---
> create-diff-object.c | 60
>
> livepatch-build | 23
> 2 files changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 1e6e617..b0b4dcb 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct
> kpatch_elf *kelf)
> }
> }
>
> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) {
> return 8; }
> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) {
> return 8; }
> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) {
> return 8; }
> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) {
> return 16; }
> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return
> 8; }
> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) {
> return 12; }
> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +static int size = 0;
> +char *str;
> +if (!size) {
> +str = getenv("BUG_STRUCT_SIZE");
> +size = str ? atoi(str) : 8;
> +}
> +
> +return size;
> +}
> +
> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +static int size = 0;
> +char *str;
> +if (!size) {
> +str = getenv("BUG_STRUCT_SIZE");
> +size = str ? atoi(str) : 16;
> +}
> +
> +return size;
> +}
> +
> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +static int size = 0;
> +char *str;
> +if (!size) {
> +str = getenv("EX_STRUCT_SIZE");
> +size = str ? atoi(str) : 8;
> +}
> +
> +return size;
> +}
> +
> +static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +static int size = 0;
> +char *str;
> +if (!size) {
> +str = getenv("ALT_STRUCT_SIZE");
> +size = str ? atoi(str) : 12;
> +}
> +
> +printf("altinstr_size=%d\n", size);
> +return size;
> +}
>
> /*
> * The rela groups in the .fixup section vary in size. The beginning of each
> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf,
> int offset)
> static struct special_section special_sections[] = {
> {
> .name = ".bug_frames.0",
> - .group_size = bug_frames_0_group_size,
> + .group_size = bug_frames_group_size,
> },
> {
> .name = ".bug_frames.1",
> - .group_size = bug_frames_1_group_size,
> + .group_size = bug_frames_group_size,
> },
> {
> .name = ".bug_frames.2",
> - .group_size = bug_frames_2_group_size,
> + .group_size = bug_frames_group_size,
> },
> {
> .name = ".bug_frames.3",
> diff --git a/livepatch-build b/livepatch-build
> index c057fa1..a6cae12 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
> echo ""
> echo
>
> +if [ -f "$XENSYMS" ]; then
> +echo "Reading special section data"
> +SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
> + gawk --non-decimal-data '
> + BEGIN { a = b = e = 0 }
> + a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next}
> + b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next}
> + e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next}
> + a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
> + b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
> + e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
> + a == 2 && b == 2 && e == 2 {exit}')
> +[[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
> +if [[ -z $ALT_STRUCT_SIZE ]] |
Re: [Xen-devel] a few xen swiotlb cleanups
On Thu, Apr 11, 2019 at 09:19:56AM +0200, Christoph Hellwig wrote: > Hi all, I will slurp these up.. right after I test them for correctness. > > below are a couple of cleanups for swiotlb-xen.c. They were done in > preparation of eventually using the dma-noncoherent.h cache flushing > hooks, but that final goal will need some major work to the arm32 code > first. Until then I think these patches might be better in mainline > than in my local tree so that they don't bitrot. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Criteria / validation proposal: drop Xen
On Fri, Apr 26, 2019 at 10:22:13PM +0530, Sumantro Mukherjee wrote: > Yup +1 from my side too. Xen is hardly tested since a lot of time. Hi! And that is thanks to one of the GRUB2 bugs that needs some love from Peter Jones. As without that bug being fixed - it is very difficult to test it - as you can't even load Xen! I've asked the upstream GRUB maintainer to sheed some light on the confusion about multiboot2 + SecureBoot - hopefully that will resolve the question. My vote is to have it remain as is. Thank you. > > On Fri, Apr 26, 2019 at 10:07 PM Geoffrey Marr wrote: > > > Since F24, I haven't seen or heard of anyone who uses Xen over KVM > > anywhere other than this thread... I'm +1 for making this test an > > "Optional" one. > > > > Geoff Marr > > IRC: coremodule > > > > > > On Fri, Apr 26, 2019 at 10:33 AM Adam Williamson < > > adamw...@fedoraproject.org> wrote: > > > >> On Thu, 2017-07-06 at 13:19 -0700, Adam Williamson wrote: > >> > On Thu, 2017-07-06 at 15:59 -0400, Konrad Rzeszutek Wilk wrote: > >> > > > > I would prefer for it to remain as it is. > >> > > > > >> > > > This is only practical if it's going to be tested, and tested > >> regularly > >> > > > - not *only* on the final release candidate, right before we sign > >> off > >> > > > on the release. It needs to be tested regularly throughout the > >> release > >> > > > cycle, on the composes that are "nominated for testing". > >> > > > >> > > Right, which is why I am happy that you have pointed me to the right > >> > > place so I can be up-to-date. > >> > > >> > Great, thanks. So let's leave it as it is for now, but we'll keep an > >> > eye on this during F27 cycle. If we get to, say, Beta and there are no > >> > results for the test, that's gonna be a problem. Thanks! > >> > >> So, for Fedora 30, this was not tested throughout the whole cycle. I > >> think we can consider the proposal to remove the criterion active > >> again. > >> -- > >> Adam Williamson > >> Fedora QA Community Monkey > >> IRC: adamw | Twitter: AdamW_Fedora | XMPP: adamw AT happyassassin . net > >> http://www.happyassassin.net > >> ___ > >> test mailing list -- t...@lists.fedoraproject.org > >> To unsubscribe send an email to test-le...@lists.fedoraproject.org > >> Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > >> List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > >> List Archives: > >> https://lists.fedoraproject.org/archives/list/t...@lists.fedoraproject.org > >> > > ___ > > test mailing list -- t...@lists.fedoraproject.org > > To unsubscribe send an email to test-le...@lists.fedoraproject.org > > Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html > > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines > > List Archives: > > https://lists.fedoraproject.org/archives/list/t...@lists.fedoraproject.org > > > > > -- > //sumantro > Fedora QE > TRIED AND PERSONALLY TESTED, ERGO TRUSTED <https://redhat.com/trusted> ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v5 00/12] livepatch: new features and fixes
On Thu, Nov 14, 2019 at 01:06:41PM +, Pawel Wieczorkiewicz wrote:
> This series introduces new features to the livepatch functionality as
> briefly discussed during Xen Developer Summit 2019: [a] and [b].
> It also provides a few fixes and some small improvements.
>
> Main changes in v4:
> - Fix various typos and minor issues
> - Simplify arch_livepatch_{apply,revert} by using
> common_livepatch_{apply,revert}
> - Improve python bindings and fix few issues
This is https://github.com/konradwilk/xen.git (your patches on top of staging):
On ARM64:
root@hikey960:/home/linaro# xl info
host : hikey960
release: 4.12.0-linaro-hikey960+
version: #3 SMP PREEMPT Mon Jul 17 13:26:13 EDT 2017
machine: aarch64
nr_cpus: 8
max_cpu_id : 7
nr_nodes : 1
cores_per_socket : 1
threads_per_core : 1
cpu_mhz: 1.920
hw_caps:
:::::::
virt_caps : hvm hap
total_memory : 2262
free_memory: 713
sharing_freed_memory : 0
sharing_used_memory: 0
outstanding_claims : 0
free_cpus : 0
xen_major : 4
xen_minor : 13
xen_extra : .0-rc
xen_version: 4.13.0-rc
xen_caps : xen-3.0-aarch64 xen-3.0-armv7l
xen_scheduler : credit2
xen_pagesize : 4096
platform_params: virt_start=0x20
xen_changeset : Thu Nov 14 13:06:52 2019 + git:9f5f25f07a
xen_commandline: console=dtuart dtuart=/soc/serial@fff32000 efi=no-rs
dom0_mem=1500M hmp-unsafe=true
cc_compiler: gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
cc_compile_by : linaro
cc_compile_domain : lan
cc_compile_date: Wed Nov 20 02:06:10 UTC 2019
build_id : 8bf9ec5fc0053f4d4fc3b7d256b66ec86f8e5ccc
xend_config_format : 4
root@hikey960:/home/linaro# cd xen.git
root@hikey960:/home/linaro/xen.git# readelf -n xen-sy
readelf: Error: 'xen-sy': No such file
root@hikey960:/home/linaro/xen.git# cd xen
root@hikey960:/home/linaro/xen.git/xen# readelf -n xen-syms
Displaying notes found in: .note.gnu.build-id
Owner Data size Description
GNU 0x0014 NT_GNU_BUILD_ID (unique build ID
bitstring)
Build ID: 8bf9ec5fc0053f4d4fc3b7d256b66ec86f8e5ccc
root@hikey960:/home/linaro/xen.git/xen# cd test/livepatch/
root@hikey960:/home/linaro/xen.git/xen/test/livepatch# xen-livepatch list
Nothing to list
root@hikey960:/home/linaro/xen.git/xen/test/livepatch# xen-livepatch load
xen_hello_world.livepatch
Uploading xen_hello_world.livepatch... completed
Applying xen_hello_world... failed
Error 22: Invalid argument
Unloading xen_hello_world... failed
Error 22: Invalid argument
root@hikey960:/home/linaro/xen.git/xen/test/livepatch# git log
commit 9f5f25f07a64e1b447f7bd124182a1c5ef422d6f
Author: Pawel Wieczorkiewicz
Date: Thu Nov 14 13:06:52 2019 +
livepatch: Add metadata runtime retrieval mechanism
...
root@hikey960:/home/linaro/xen.git/xen/test/livepatch#xl dmesg -c
(XEN) Checking for initrd in /chosen
(XEN) RAM: - 1abf
(XEN) RAM: 1ad88000 - 31ff
(XEN) RAM: 32101000 - 3dff
(XEN) RAM: 4000 - 5af25fff
(XEN) RAM: 89cc - b8767fff
(XEN) RAM: b9ac - b9ac8fff
(XEN) RAM: b9bfb000 - b9cb
(XEN) RAM: b9d62000 - b9e0
(XEN) RAM: ba1d - ba1dbfff
(XEN) RAM: ba1dc000 - bdc46fff
(XEN) RAM: bdc47000 - bdd06fff
(XEN) RAM: bdd07000 - bddd6fff
(XEN) RAM: bddd7000 - bf00
(XEN) RAM: bf01 - bf012fff
(XEN) RAM: bf013000 - bf19
(XEN) RAM: bf1a - bf1e
(XEN) RAM: bf24 - bf24efff
(XEN) RAM: bf24f000 - bfff
(XEN)
(XEN) MODULE[0]: b8773000 - b88be900 Xen
(XEN) MODULE[1]: b8768000 - b8773000 Device Tree
(XEN) MODULE[2]: b88c9000 - b9885a00 Kernel
(XEN) RESVD[0]: 3200 - 320f
(XEN)
(XEN) CMDLINE[b88c9000]:chosen console=tty0 console=hvc0
root=/dev/sdd10 rw efi=noruntime
(XEN)
(XEN) Command line: console=dtuart dtuart=/soc/serial@fff32000 efi=no-rs
dom0_mem=1500M hmp-unsafe=true
(XEN) parameter "efi" unknown!
(XEN) Domain heap initialised
(XEN) Booting using Device Tree
(XEN) Platform: Generic System
(XEN) Looking for dtuart at "/soc/serial@fff32000", options ""
Xen 4.13.0-rc
(XEN) Xen version 4.13.0-rc (linaro@lan) (gcc (Debian 6.3.0-18+deb9u1) 6.3.0
20170516) debug=y Wed Nov 20 02:06:10 UTC 2019
(XEN) Latest ChangeSet: Thu Nov 14 13:06:52 2019 + git:9f5f25f07a
(XEN) build-id: 8bf9ec5fc0053f4d4fc3b7d256b66e
Re: [Xen-devel] [PATCH v5 00/12] livepatch: new features and fixes
> Yes, this hunk is missing (somehow it did not make it to the v5 patchset, > sorry): > > diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c > index 7747ea83aa..0b21a6aca4 100644 > --- a/tools/libxc/xc_misc.c > +++ b/tools/libxc/xc_misc.c > @@ -976,6 +976,7 @@ static int _xc_livepatch_action(xc_interface *xch, > sysctl.u.livepatch.u.action.cmd = action; > sysctl.u.livepatch.u.action.timeout = timeout; > sysctl.u.livepatch.u.action.flags = flags; > +sysctl.u.livepatch.u.action.pad = 0; > > sysctl.u.livepatch.u.action.name = def_name; > set_xen_guest_handle(sysctl.u.livepatch.u.action.name.name, name); That did it! With that I can test the livepatches on ARM[32,64]. Let me squash that in "livepatch: Allow to override inter-modules buildid dependency" See: https://github.com/konradwilk/xen.git #livepatch.aws.v5 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
> > I plan to release ack the patch in case the missing maintainer's acks > > are not coming in too late. > > I think Andy's objection was that there has been zero testing of > livepatching on gcc. Maybe we can find someone to do a smoke-test. As in integrate livepatch-build tools in osstest smoke-tests? Because the livepatch test cases are in osstest, unless something went awry? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
On Mon, Dec 02, 2019 at 03:55:04PM +, Andrew Cooper wrote: > On 02/12/2019 15:53, Konrad Rzeszutek Wilk wrote: > >>> I plan to release ack the patch in case the missing maintainer's acks > >>> are not coming in too late. > >> I think Andy's objection was that there has been zero testing of > >> livepatching on gcc. Maybe we can find someone to do a smoke-test. > > As in integrate livepatch-build tools in osstest smoke-tests? > > Because the livepatch test cases are in osstest, unless something went awry? > > The sum total of livepatch testing in OSSTest is using the hand-coded > ELF objects from the tests/ directory. > > This is perhaps ok for the basic mechanism, but its not representative > of actually building real livepatches using livepatch build tools. True. But it tests the _hypervisor_ livepatch code. I am thinking that this discussion about "oh, but livepatch-build tools don't work b/c" is well sucks but should never block an release as the core livepatch functionality is OK. Irrespective of that the testing of livepatch-build tools should be in osstest, granted nobody has taken a step in this - but is somebody signing up for it? [I can't, -ENOTIME] > > ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] livepatch: Fix typos and other errors in tests Makefile
On Fri, Dec 20, 2019 at 07:15:33PM +, Julien Grall wrote: > Hi Pawel, > > Thank you for fixing the build. > > On 20/12/2019 18:23, Pawel Wieczorkiewicz wrote: > > There was a bunch of typos (s/actions/action/) as well as one missing > > config.h target dependency. Also, xen_expectation target has > > unnecessary cycle dependency. > > I would suggest to mention which commit your are fixing. I guess there are > multiple ones, but we could just mention the merge (it is at least useful > for something!). > > I guess this could be fixed on commit. > > > > > Signed-off-by: Pawel Wieczorkiewicz > > Tested-by: Julien Grall Reviewed-by: Konrad Rzeszutek Wilk But I see it is already checked in. Thank you for fixing that! > > Cheers, > > -- > Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/3] x86/boot: Introduce the kernel_info
> >>> +be prefixed with header/magic and its size, e.g.: > >>> + > >>> + kernel_info: > >>> + .ascii "LToP" /* Header, Linux top (structure). */ > >>> + .long kernel_info_var_len_data - kernel_info > >>> + .long kernel_info_end - kernel_info > >>> + .long 0x01234567 /* Some fixed size data for the > >>> bootloaders. */ > >>> + kernel_info_var_len_data: > >>> + example_struct: /* Some variable size data for the > >>> bootloaders. */ > >>> + .ascii "EsTT" /* Header/Magic. */ > >>> + .long example_struct_end - example_struct > >>> + .ascii "Struct" > >>> + .long 0x89012345 > >>> + example_struct_end: > >>> + example_strings:/* Some variable size data for the > >>> bootloaders. */ > >>> + .ascii "EsTs" /* Header/Magic. */ > >> > >> Where do the Magic values "EsTT" and "EsTs" come from? > >> where are they defined? > > > > EsTT == Example STrucT > > EsTs == Example STringS > > > > Anyway, it can be anything which does not collide with existing variable > > length data magics. There are none right now. So, it can be anything. > > Maybe I should add something saying that. > > Yes, please. Or make it very clear they are examples, says "1234" or "ABCD" or such. > > thanks. > -- > ~Randy ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 00/12] livepatch: new features and fixes
On Sat, Sep 28, 2019 at 03:12:53PM +, Pawel Wieczorkiewicz wrote: > This series introduces new features to the livepatch functionality as > briefly discussed during Xen Developer Summit 2019: [a] and [b]. > It also provides a few fixes and some small improvements. Heya, Is there an v5 of the patches somewhere brewing so that I can give them one last test? Juergen, Are you OK with giving them an Release-Ack? I think there are only two minor changes that Jan requested. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] x86/livepatch: Fail the build if duplicate symbols exist
On October 23, 2019 10:46:37 AM EDT, "Jürgen Groß" wrote: >On 23.10.19 15:58, Andrew Cooper wrote: >> From: Ross Lagerwall >> >> The binary diffing algorithm used by xen-livepatch depends on having >unique >> symbols. >> >> Signed-off-by: Ross Lagerwall >> >> The livepatch loading algorithm used by Xen resolves relocations by >symbol >> name, and thus also depends on having unique symbols. >> >> Introduce CONFIG_ENFORCE_UNIQUE_SYMBOLS to control failing the build >if >> duplicate symbols are found, and disable it in the RANDCONFIG build. >> >> Signed-off-by: Andrew Cooper What is up with that SoBs not being together? Could you squash them please? Patch wise, feel free to add my Reviewed-by. Thx > >Release-acked-by: Juergen Gross > > >Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/livepatch: Add a return value to load hooks
On Tue, Nov 05, 2019 at 07:43:16PM +, Andrew Cooper wrote:
> One use of load hooks is to perform a safety check of the system in its
> quiesced state. Any non-zero return value from a load hook will abort the
> apply attempt.
>
Shouldn't you also update the documentation (design doc?)
Thanks.
> Signed-off-by: Andrew Cooper
> ---
> CC: Konrad Rzeszutek Wilk
> CC: Ross Lagerwall
> CC: Juergen Gross
>
> For several years, the following patch in the series has been shipped in every
> XenServer livepatch, implemented as a void load hook which modifies its return
> address to skip to the end of apply_payload(). This method is distinctly less
> hacky.
> ---
> xen/common/livepatch.c | 30 +++---
> xen/include/xen/livepatch_payload.h | 2 +-
> xen/test/livepatch/xen_hello_world.c | 12 +---
> 3 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 7caa30c202..962647616a 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1076,25 +1076,33 @@ static int apply_payload(struct payload *data)
> * temporarily disable the spin locks IRQ state checks.
> */
> spin_debug_disable();
> -for ( i = 0; i < data->n_load_funcs; i++ )
> -data->load_funcs[i]();
> +for ( i = 0; !rc && i < data->n_load_funcs; i++ )
> +rc = data->load_funcs[i]();
> spin_debug_enable();
>
> +if ( rc )
> +printk(XENLOG_ERR LIVEPATCH "%s: load_funcs[%u] failed: %d\n",
> + data->name, i, rc);
> +
> ASSERT(!local_irq_is_enabled());
>
> -for ( i = 0; i < data->nfuncs; i++ )
> -arch_livepatch_apply(&data->funcs[i]);
> +if ( !rc )
> +for ( i = 0; i < data->nfuncs; i++ )
> +arch_livepatch_apply(&data->funcs[i]);
>
> arch_livepatch_revive();
>
> -/*
> - * We need RCU variant (which has barriers) in case we crash here.
> - * The applied_list is iterated by the trap code.
> - */
> -list_add_tail_rcu(&data->applied_list, &applied_list);
> -register_virtual_region(&data->region);
> +if ( !rc )
> +{
> +/*
> + * We need RCU variant (which has barriers) in case we crash here.
> + * The applied_list is iterated by the trap code.
> + */
> +list_add_tail_rcu(&data->applied_list, &applied_list);
> +register_virtual_region(&data->region);
> +}
>
> -return 0;
> +return rc;
> }
>
> static int revert_payload(struct payload *data)
> diff --git a/xen/include/xen/livepatch_payload.h
> b/xen/include/xen/livepatch_payload.h
> index 4a1a96d054..3788b52ddf 100644
> --- a/xen/include/xen/livepatch_payload.h
> +++ b/xen/include/xen/livepatch_payload.h
> @@ -9,7 +9,7 @@
> * The following definitions are to be used in patches. They are taken
> * from kpatch.
> */
> -typedef void livepatch_loadcall_t(void);
> +typedef int livepatch_loadcall_t(void);
> typedef void livepatch_unloadcall_t(void);
>
> /*
> diff --git a/xen/test/livepatch/xen_hello_world.c
> b/xen/test/livepatch/xen_hello_world.c
> index 02f3f85dc0..d720001f07 100644
> --- a/xen/test/livepatch/xen_hello_world.c
> +++ b/xen/test/livepatch/xen_hello_world.c
> @@ -16,9 +16,11 @@ static const char hello_world_patch_this_fnc[] =
> "xen_extra_version";
> extern const char *xen_hello_world(void);
> static unsigned int cnt;
>
> -static void apply_hook(void)
> +static int apply_hook(void)
> {
> printk(KERN_DEBUG "Hook executing.\n");
> +
> +return 0;
> }
>
> static void revert_hook(void)
> @@ -26,10 +28,14 @@ static void revert_hook(void)
> printk(KERN_DEBUG "Hook unloaded.\n");
> }
>
> -static void hi_func(void)
> +static int hi_func(void)
> {
> printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
> +
> +return 0;
> };
> +/* Safe to cast away the return value for this trivial case. */
> +void (void_hi_func)(void) __attribute__((alias("hi_func")));
>
> static void check_fnc(void)
> {
> @@ -43,7 +49,7 @@ LIVEPATCH_UNLOAD_HOOK(revert_hook);
> /* Imbalance here. Two load and three unload. */
>
> LIVEPATCH_LOAD_HOOK(hi_func);
> -LIVEPATCH_UNLOAD_HOOK(hi_func);
> +LIVEPATCH_UNLOAD_HOOK(void_hi_func);
>
> LIVEPATCH_UNLOAD_HOOK(check_fnc);
>
> --
> 2.11.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] x86/livepatch: Prevent patching with active waitqueues
On Tue, Nov 05, 2019 at 07:43:17PM +, Andrew Cooper wrote:
> The safety of livepatching depends on every stack having been unwound, but
> there is one corner case where this is not true. The Sharing/Paging/Monitor
> infrastructure may use waitqueues, which copy the stack frame sideways and
> longjmp() to a different vcpu.
>
> This case is rare, and can be worked around by pausing the offending
> domain(s), waiting for their rings to drain, then performing a livepatch.
>
> In the case that there is an active waitqueue, fail the livepatch attempt with
> -EBUSY, which is preforable to the fireworks which occur from trying to unwind
> the old stack frame at a later point.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Konrad Rzeszutek Wilk
Reviewed-by: Konrad Rzeszutek Wilk
> CC: Ross Lagerwall
> CC: Juergen Gross
>
> This fix wants backporting, and is long overdue for posting upstream.
> ---
> xen/arch/arm/livepatch.c| 5 +
> xen/arch/x86/livepatch.c| 39 +++
> xen/common/livepatch.c | 7 +++
> xen/include/xen/livepatch.h | 1 +
> 4 files changed, 52 insertions(+)
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 00c5e2bc45..915e9d926a 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -18,6 +18,11 @@
>
> void *vmap_of_xen_text;
>
> +int arch_livepatch_safety_check(void)
> +{
> +return 0;
> +}
> +
> int arch_livepatch_quiesce(void)
> {
> mfn_t text_mfn;
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index c82cf53b9e..0f129fa6b2 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -14,6 +14,45 @@
> #include
> #include
>
> +static bool has_active_waitqueue(const struct vm_event_domain *ved)
> +{
> +/* ved may be xzalloc()'d without INIT_LIST_HEAD() yet. */
> +return (ved && !list_head_is_null(&ved->wq.list) &&
> +!list_empty(&ved->wq.list));
> +}
> +
> +/*
> + * x86's implementation of waitqueue violates the livepatching safey
> principle
> + * of having unwound every CPUs stack before modifying live content.
> + *
> + * Search through every domain and check that no vCPUs have an active
> + * waitqueue.
> + */
> +int arch_livepatch_safety_check(void);
> +{
> +struct domain *d;
> +
> +for_each_domain ( d )
> +{
> +#ifdef CONFIG_MEM_SHARING
> +if ( has_active_waitqueue(d->vm_event_share) )
> +goto fail;
> +#endif
> +#ifdef CONFIG_MEM_PAGING
> +if ( has_active_waitqueue(d->vm_event_paging) )
> +goto fail;
> +#endif
> +if ( has_active_waitqueue(d->vm_event_monitor) )
> +goto fail;
> +}
> +
> +return 0;
> +
> + fail:
> +printk(XENLOG_ERR LIVEPATCH "%pd found with active waitqueue\n", d);
> +return -EBUSY;
> +}
> +
> int arch_livepatch_quiesce(void)
> {
> /* Disable WP to allow changes to read-only pages. */
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 962647616a..27ee5bdeb7 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1060,6 +1060,13 @@ static int apply_payload(struct payload *data)
> unsigned int i;
> int rc;
>
> +rc = apply_safety_checks();
> +if ( rc )
> +{
> +printk(XENLOG_ERR LIVEPATCH "%s: Safety checks failed\n",
> data->name);
> +return rc;
> +}
> +
> printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
> data->name, data->nfuncs);
>
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 1b1817ca0d..69ede75d20 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -104,6 +104,7 @@ static inline int livepatch_verify_distance(const struct
> livepatch_func *func)
> * These functions are called around the critical region patching live code,
> * for an architecture to take make appropratie global state adjustments.
> */
> +int arch_livepatch_safety_check(void);
> int arch_livepatch_quiesce(void);
> void arch_livepatch_revive(void);
>
> --
> 2.11.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names
On Fri, Nov 08, 2019 at 12:18:40PM +0100, Jan Beulich wrote:
> The .file assembler directives generated by the compiler do not include
> any path components (gcc) or just the ones specified on the command line
> (clang, at least version 5), and hence multiple identically named source
> files (in different directories) may produce identically named static
> symbols (in their kallsyms representation). The binary diffing algorithm
> used by xen-livepatch, however, depends on having unique symbols.
>
> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
> behavior, and if enabled use objcopy to prepend the (relative to the
> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
> that this build option is made no longer depend on LIVEPATCH, but merely
> defaults to its setting now.
>
> Conditionalize explicit .file directive insertion in C files where it
> exists just to disambiguate names in a less generic manner; note that
> at the same time the redundant emission of STT_FILE symbols gets
> suppressed for clang. Assembler files as well as multiply compiled C
> ones using __OBJECT_FILE__ are left alone for the time being.
>
> Since we now expect there not to be any duplicates anymore, also don't
> force the selection of the option to 'n' anymore in allrandom.config.
> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
> enforcement is in effect, which in turn allows
> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
> !ENFORCE_UNIQUE_SYMBOLS.
>
> Signed-off-by: Jan Beulich
Acked-by: Konrad Rzeszutek Wilk
> ---
> v2: Re-base. Conditionalize COVERAGE's select.
>
> The clang behavior may require further tweaking if different versions
> behave differently. Alternatively we could pass two --redefine-sym
> arguments to objcopy.
>
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -38,7 +38,7 @@ config FRAME_POINTER
> config COVERAGE
> bool "Code coverage support"
> depends on !LIVEPATCH
> - select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> + select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS if !ENFORCE_UNIQUE_SYMBOLS
> ---help---
> Enable code coverage support.
>
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -194,12 +194,24 @@ FORCE:
>
> .PHONY: clean
> clean:: $(addprefix _clean_, $(subdir-all))
> - rm -f *.o *~ core $(DEPS_RM)
> + rm -f *.o .*.o.tmp *~ core $(DEPS_RM)
> _clean_%/: FORCE
> $(MAKE) -f $(BASEDIR)/Rules.mk -C $* clean
>
> +SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR))
> +
> %.o: %.c Makefile
> +ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
> + $(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp
> +ifeq ($(clang),y)
> + $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
> +else
> + $(OBJCOPY) --redefine-sym $( +endif
> + rm -f $(@D)/.$(@F).tmp
> +else
> $(CC) $(CFLAGS) -c $< -o $@
> +endif
>
> %.o: %.S Makefile
> $(CC) $(AFLAGS) -c $< -o $@
> --- a/xen/arch/x86/x86_64/compat.c
> +++ b/xen/arch/x86/x86_64/compat.c
> @@ -2,7 +2,7 @@
> * compat.c
> */
>
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>
> #include
> #include
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -16,7 +16,7 @@
> * with this program; If not, see <http://www.gnu.org/licenses/>.
> */
>
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>
> #include
> #include
> --- a/xen/arch/x86/x86_64/physdev.c
> +++ b/xen/arch/x86/x86_64/physdev.c
> @@ -2,7 +2,7 @@
> * physdev.c
> */
>
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>
> #include
> #include
> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -2,7 +2,7 @@
> * platform_hypercall.c
> */
>
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>
> #include
> #include
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -373,8 +373,7 @@ config FAST_SYMBOL_LOOKUP
>
> config ENFORCE_UNIQUE_SYMBOLS
> bool "Enforce unique symbols"
> - default y
> - depends on LIVEPATCH
> + default LIVEPATCH
> ---help---
> Multiple symbols with the same name aren't generally a problem
> unless livepatching is to be used.
> @@ -387,8 +386,8 @@ config ENFORCE_UNIQUE_SYMBOLS
> livepatch build and apply correctly.
>
> config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> - bool "Suppress duplicate symbol warnings"
Re: [Xen-devel] [PATCH v2 0/3] Remove tmem
On Wed, Nov 28, 2018 at 01:58:03PM +, Wei Liu wrote: > It is agreed that tmem can be removed from xen.git. See the thread starting > > > from . > > In this version: > > 1. Remove some residuals from previous version and fix all build errors >discovered by Gitlab CI. > 2. Swap the order of patches to make sure bisection still works. This >is verified by calling > `./automation/scripts/build-test.sh origin/staging HEAD` > 3. Make sure Xen still boots and passes all XTF tests after the removal. > 4. Keep public/tmem.h. Please also remove the entry in the MAINTAINERS file. Acked-by: Konrad Rzeszutek Wilk ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 21/23] xen-swiotlb: remove the mapping_error dma_map_ops method
On Fri, Nov 30, 2018 at 02:22:29PM +0100, Christoph Hellwig wrote:
> Return DMA_MAPPING_ERROR instead of 0 on a dma mapping failure and let
> the core dma-mapping code handle the rest.
>
> Signed-off-by: Christoph Hellwig
Reviewed-by: Konrad Rzeszutek Wilk
> ---
> drivers/xen/swiotlb-xen.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 2a7f545bd0b5..6dc969d5ea2f 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -53,8 +53,6 @@
> * API.
> */
>
> -#define XEN_SWIOTLB_ERROR_CODE (~(dma_addr_t)0x0)
> -
> static char *xen_io_tlb_start, *xen_io_tlb_end;
> static unsigned long xen_io_tlb_nslabs;
> /*
> @@ -406,7 +404,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
>attrs);
> if (map == SWIOTLB_MAP_ERROR)
> - return XEN_SWIOTLB_ERROR_CODE;
> + return DMA_MAPPING_ERROR;
>
> dev_addr = xen_phys_to_bus(map);
> xen_dma_map_page(dev, pfn_to_page(map >> PAGE_SHIFT),
> @@ -421,7 +419,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> *dev, struct page *page,
> attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> swiotlb_tbl_unmap_single(dev, map, size, dir, attrs);
>
> - return XEN_SWIOTLB_ERROR_CODE;
> + return DMA_MAPPING_ERROR;
> }
>
> /*
> @@ -700,11 +698,6 @@ xen_swiotlb_get_sgtable(struct device *dev, struct
> sg_table *sgt,
> return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size, attrs);
> }
>
> -static int xen_swiotlb_mapping_error(struct device *dev, dma_addr_t dma_addr)
> -{
> - return dma_addr == XEN_SWIOTLB_ERROR_CODE;
> -}
> -
> const struct dma_map_ops xen_swiotlb_dma_ops = {
> .alloc = xen_swiotlb_alloc_coherent,
> .free = xen_swiotlb_free_coherent,
> @@ -719,5 +712,4 @@ const struct dma_map_ops xen_swiotlb_dma_ops = {
> .dma_supported = xen_swiotlb_dma_supported,
> .mmap = xen_swiotlb_dma_mmap,
> .get_sgtable = xen_swiotlb_get_sgtable,
> - .mapping_error = xen_swiotlb_mapping_error,
> };
> --
> 2.19.1
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1] xen-blkfront: dynamic configuration of per-vbd resources
From: Bob Liu
The current VBD layer reserves buffer space for each attached device based on
three statically configured settings which are read at boot time.
* max_indirect_segs: Maximum amount of segments.
* max_ring_page_order: Maximum order of pages to be used for the shared ring.
* max_queues: Maximum of queues(rings) to be used.
But the storage backend, workload, and guest memory result in very different
tuning requirements. It's impossible to centrally predict application
characteristics so it's best to leave allow the settings can be dynamiclly
adjusted based on workload inside the Guest.
Usage:
Show current values:
cat /sys/devices/vbd-xxx/max_indirect_segs
cat /sys/devices/vbd-xxx/max_ring_page_order
cat /sys/devices/vbd-xxx/max_queues
Write new values:
echo > /sys/devices/vbd-xxx/max_indirect_segs
echo > /sys/devices/vbd-xxx/max_ring_page_order
echo > /sys/devices/vbd-xxx/max_queues
Signed-off-by: Bob Liu
Signed-off-by: Somasundaram Krishnasamy
Signed-off-by: Konrad Rzeszutek Wilk
---
drivers/block/xen-blkfront.c | 320 ---
1 file changed, 304 insertions(+), 16 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 92ec1bbece51..4ebd368f4d1a 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -46,6 +46,7 @@
#include
#include
#include
+#include
#include
#include
@@ -217,6 +218,11 @@ struct blkfront_info
/* Save uncomplete reqs and bios for migration. */
struct list_head requests;
struct bio_list bio_list;
+ /* For dynamic configuration. */
+ unsigned int reconfiguring:1;
+ int new_max_indirect_segments;
+ int new_max_ring_page_order;
+ int new_max_queues;
};
static unsigned int nr_minors;
@@ -1355,6 +1361,31 @@ static void blkif_free(struct blkfront_info *info, int
suspend)
for (i = 0; i < info->nr_rings; i++)
blkif_free_ring(&info->rinfo[i]);
+ /* Remove old xenstore nodes. */
+ if (info->nr_ring_pages > 1)
+ xenbus_rm(XBT_NIL, info->xbdev->nodename, "ring-page-order");
+
+ if (info->nr_rings == 1) {
+ if (info->nr_ring_pages == 1) {
+ xenbus_rm(XBT_NIL, info->xbdev->nodename, "ring-ref");
+ } else {
+ for (i = 0; i < info->nr_ring_pages; i++) {
+ char ring_ref_name[RINGREF_NAME_LEN];
+
+ snprintf(ring_ref_name, RINGREF_NAME_LEN,
"ring-ref%u", i);
+ xenbus_rm(XBT_NIL, info->xbdev->nodename,
ring_ref_name);
+ }
+ }
+ } else {
+ xenbus_rm(XBT_NIL, info->xbdev->nodename,
"multi-queue-num-queues");
+
+ for (i = 0; i < info->nr_rings; i++) {
+ char queuename[QUEUE_NAME_LEN];
+
+ snprintf(queuename, QUEUE_NAME_LEN, "queue-%u", i);
+ xenbus_rm(XBT_NIL, info->xbdev->nodename, queuename);
+ }
+ }
kfree(info->rinfo);
info->rinfo = NULL;
info->nr_rings = 0;
@@ -1778,10 +1809,18 @@ static int talk_to_blkback(struct xenbus_device *dev,
if (!info)
return -ENODEV;
- max_page_order = xenbus_read_unsigned(info->xbdev->otherend,
- "max-ring-page-order", 0);
- ring_page_order = min(xen_blkif_max_ring_order, max_page_order);
- info->nr_ring_pages = 1 << ring_page_order;
+ err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
+ "max-ring-page-order", "%u", &max_page_order);
+ if (err != 1)
+ info->nr_ring_pages = 1;
+ else {
+ ring_page_order = min(xen_blkif_max_ring_order, max_page_order);
+ if (info->new_max_ring_page_order) {
+ BUG_ON(info->new_max_ring_page_order > max_page_order);
+ ring_page_order = info->new_max_ring_page_order;
+ }
+ info->nr_ring_pages = 1 << ring_page_order;
+ }
err = negotiate_mq(info);
if (err)
@@ -1903,6 +1942,10 @@ static int negotiate_mq(struct blkfront_info *info)
backend_max_queues = xenbus_read_unsigned(info->xbdev->otherend,
"multi-queue-max-queues", 1);
info->nr_rings = min(backend_max_queues, xen_blkif_max_queues);
+ if (info->new_max_queues) {
+ BUG_ON(info->new_max_queues > backend_max_queues);
+ info->nr_rings = info->new_max_queues;
+ }
/* We need at least one r
[Xen-devel] [PATCH v1] Xen-blkfront fixes to dynamically adjust ring.
Hi! This patch allows dynamic reconfiguration of the three different parameters that an Xen blkfront driver initially negotiates: * max_indirect_segs: Maximum amount of segments. * max_ring_page_order: Maximum order of pages to be used for the shared ring. * max_queues: Maximum of queues(rings) to be used. But the storage backend, workload, and guest memory result in very different tuning requirements. It's impossible to centrally predict application characteristics so it's best to leave allow the settings can be dynamiclly adjusted based on workload inside the guest. drivers/block/xen-blkfront.c | 320 --- 1 file changed, 304 insertions(+), 16 deletions(-) Bob Liu (1): xen-blkfront: dynamic configuration of per-vbd resources ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Resume from suspend to RAM broken when using early microcode updates
On Wed, Apr 11, 2018 at 01:12:51PM +0100, Andrew Cooper wrote: > On 11/04/18 13:01, Simon Gaiser wrote: > > Andrew Cooper: > >> On 11/04/18 12:48, Simon Gaiser wrote: > >>> Hi, > >>> > >>> when I use early microcode loading with the microcode update with the > >>> BTI mitigations, resuming from suspend to RAM is broken. > >>> > >>> Based on added logging to enter_state() (from power.c) it doesn't > >>> survive the local_irq_restore(flags) call (at least a printk() after the > >>> call doesn't output anything on the serial console). > >>> > >>> I guess that some irq handler tries to use IBRS/IBPB. But the microcode > >>> is only loaded later. > >>> > >>> If I simply move the microcode_resume_cpu(0) directly before the > >>> local_irq_restore(flags) everything seems to work fine. But I'm not sure > >>> if this has unintended consequences. > >>> > >>> I tested the above with Xen 4.8.3 from Qubes which includes the BTI and > >>> microcode patches from staging-4.8. AFAICS there are no commits which > >>> changes the affected code or other commits which sound relevant so this > >>> probably affected also all the newer branches. > >> S3 support is a very unloved area of the hypervisor. > >> > >> Yes - we definitely need to get microcode reloaded before interrupts are > >> enabled. > > Do you see any problems with simply moving microcode_resume_cpu(0) > > directly before the local_irq_restore(flags) call? (I'm not familiar > > with the code at all and (early) resume handling sounds like something > > which is easy to break in non obvious ways) > > Judging by what is going on, it wants to be between tboot_s3_error() and > the done label. > > We only need to restore microcode if we successfully went into S3. The > done and enable_cpu labels are only used by paths which don't need to > restore microcode. > > OTOH, you should check the return value and panic if restoration > failed. As you've seen, the system won't survive trying to blindly > continue resuming. > > > > >> That said, I would have expected a backtrace complaining about > >> a GP fault if we had hit the use of IBRS/IBPB before the microcode was > >> reloaded. > > Yeah, not sure what's happening here. I don't get any output from after > > local_irq_restore(flags). If you have some ideas for more debug output I > > can easily test it. > > In hindsight, I am. We take a #GP fault because of a bad MSR, and at > the head of the exception handler try to use the same bad MSR. It will > repeatedly fault until hitting a guard page (or other read-only page), > at which point we take a double fault, and suffer a #GP yet again. > Taking a #DF will reset the stack to a moderately sane value, and the > system will livelock taking faults. > > This is an unfortunate consequence of having $MAGIC in the exception > handlers. We can just disable IBRS before going to sleep? > > ~Andrew > > ___ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 0/5] sndif: add explicit back and front synchronization
On Wed, Mar 21, 2018 at 09:15:36AM +0200, Oleksandr Andrushchenko wrote: > On 03/20/2018 10:22 PM, Takashi Iwai wrote: > > On Mon, 19 Mar 2018 08:22:19 +0100, > > Oleksandr Andrushchenko wrote: > > > From: Oleksandr Andrushchenko > > > > > > Hello, all! > > > > > > In order to provide explicit synchronization between backend and > > > frontend the following changes are introduced in the protocol: > > > - bump protocol version to 2 > > > - add new ring buffer for sending asynchronous events from > > > backend to frontend to report number of bytes played by the > > > frontend (XENSND_EVT_CUR_POS) > > > - introduce trigger events for playback control: start/stop/pause/resume > > > - add "req-" prefix to event-channel and ring-ref to unify naming > > > of the Xen event channels for requests and events > > > - add XENSND_OP_HW_PARAM_QUERY request to read/update > > > stream configuration space: request passes desired intervals/formats > > > for > > > the stream parameters and the response returns allowed intervals and > > > formats mask that can be used. > > > > > > Changes since v2: > > > 1. Konrad's r-b tag for version patch > > > 2. MAJOR: changed req/resp/evt packet sizes from 32 to 64 octets > > > 3. Reworked XENSND_OP_HW_PARAM_QUERY so it now sends all > > > parameters at once, allowing to check all the configuration > > > space. > > > 4. Minor documentation cleanup (added missed "reserved" fields) > > > > > > Changes since v1: > > > > > > 1. Changed protocol version definition from string to integer, > > > so it can easily be used in comparisons. > > > Konrad, I have removed your r-b tag for the reason of this change. > > > > > > 2. In order to provide explicit stream parameter negotiation between > > > backend and frontend the following changes are introduced in the protocol: > > > add XENSND_OP_HW_PARAM_QUERY request to read/update > > > configuration space for the parameter given: request passes > > > desired parameter interval (mask) and the response to this request > > > returns min/max interval (mask) for the parameter to be used. > > > > > > Parameters supported by this request/response: > > > - format mask > > > - sample rate interval > > > - number of channels interval > > > - buffer size, interval, frames > > > - period size, interval, frames > > I can't judge exactly about the protocol without the actual FE/BE > > implementations, but the change looks good to me, especially if you've > > already tested something. > Thank you, I have tested the changes and need them to start upstreaming > the frontend driver used to test the protocol. > Do you mind if I put your Acked-by (or you prefer Reviewed-by?) tag to these > patches: > > [PATCH v3 4/5] sndif: Add explicit back and front synchronization > [PATCH v3 5/5] sndif: Add explicit back and front parameter negotiation > > Please note, that the changes first to be merged into Xen and then I'll > prepare > the same, but for the kernel > > > > If other people have no concern, let's go ahead with FE/BE stuff. > Konrad, are you ok with the changes? Yes. Thank you for your persistence. Can you also add: Reviewed-by: Konrad Rzeszutek Wilk Thank you! > > > > thanks, > > > > Takashi > Thank you, > Oleksandr ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RESEND] xen/sndif: Sync up with the canonical definition in Xen
On Thu, Apr 12, 2018 at 01:46:33PM -0400, Boris Ostrovsky wrote: > On 04/12/2018 01:26 PM, Oleksandr Andrushchenko wrote: > > This is the sync up with the canonical definition of the sound > > protocol in Xen: > > > > 1. Protocol version was referenced in the protocol description, > >but missed its definition. Fixed by adding a constant > >for current protocol version. > > > > 2. Some of the request descriptions have "reserved" fields > >missed: fixed by adding corresponding entries. > > > > 3. Extend the size of the requests and responses to 64 octets. > >Bump protocol version to 2. > > > > 4. Add explicit back and front synchronization > >In order to provide explicit synchronization between backend and > >frontend the following changes are introduced in the protocol: > > - add new ring buffer for sending asynchronous events from > > backend to frontend to report number of bytes played by the > > frontend (XENSND_EVT_CUR_POS) > > - introduce trigger events for playback control: start/stop/pause/resume > > - add "req-" prefix to event-channel and ring-ref to unify naming > > of the Xen event channels for requests and events > > > > 5. Add explicit back and front parameter negotiation > >In order to provide explicit stream parameter negotiation between > >backend and frontend the following changes are introduced in the > > protocol: > >add XENSND_OP_HW_PARAM_QUERY request to read/update > >configuration space for the parameters given: request passes > >desired parameter's intervals/masks and the response to this request > >returns allowed min/max intervals/masks to be used. > > > > Signed-off-by: Oleksandr Andrushchenko > > Signed-off-by: Oleksandr Grytsov > > Cc: Konrad Rzeszutek Wilk > > Cc: Takashi Iwai > > --- > > Reviewed-by: Boris Ostrovsky > Reviewed-by: Konrad Rzeszutek Wilk Thank you! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/8] x86/HVM/SVM: Add AVIC initialization code
> +++ b/xen/arch/x86/hvm/svm/avic.c > @@ -0,0 +1,191 @@ > +/* > + * avic.c: implements AMD Advanced Virtual Interrupt Controller (AVIC) > support > + * Copyright (c) 2016, Advanced Micro Devices, Inc. Not 2018? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.11] x86/spec_ctrl: Updates to retpoline-safety decision making
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c > index 5b5ec90..aff06f0 100644 > --- a/xen/arch/x86/spec_ctrl.c > +++ b/xen/arch/x86/spec_ctrl.c > @@ -113,12 +113,13 @@ static void __init print_details(enum ind_thunk thunk) > printk(XENLOG_DEBUG "Speculative mitigation facilities:\n"); > > /* Hardware features which pertain to speculative mitigations. */ > -printk(XENLOG_DEBUG " Hardware features:%s%s%s%s%s\n", > +printk(XENLOG_DEBUG " Hardware features:%s%s%s%s%s%s\n", > (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "", > (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "", > (e8b & cpufeat_mask(X86_FEATURE_IBPB)) ? " IBPB" : "", > (caps & ARCH_CAPABILITIES_IBRS_ALL) ? " IBRS_ALL" : "", > - (caps & ARCH_CAPABILITIES_RDCL_NO) ? " RDCL_NO" : ""); > + (caps & ARCH_CAPABILITIES_RDCL_NO) ? " RDCL_NO" : "", > + (caps & ARCH_CAPS_RSBA) ? " RBSA" : ""); You have RSBA on the left and RBSA on the right. Which one is it? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.11] x86/spec_ctrl: Fix typo in ARCH_CAPS decode
On April 24, 2018 5:44:33 AM EDT, Andrew Cooper wrote: >Signed-off-by: Andrew Cooper >--- >CC: Jan Beulich >CC: Juergen Gross You are missing an Reported-by.. Also pls Reviewed-by: Konrad Rzeszutek Wilk Thx >--- > xen/arch/x86/spec_ctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c >index bab8595..fab3c1d 100644 >--- a/xen/arch/x86/spec_ctrl.c >+++ b/xen/arch/x86/spec_ctrl.c >@@ -119,7 +119,7 @@ static void __init print_details(enum ind_thunk >thunk) > (e8b & cpufeat_mask(X86_FEATURE_IBPB)) ? " IBPB" : "", > (caps & ARCH_CAPABILITIES_IBRS_ALL) ? " IBRS_ALL" : "", > (caps & ARCH_CAPABILITIES_RDCL_NO) ? " RDCL_NO" : "", >- (caps & ARCH_CAPS_RSBA) ? " RBSA" : >""); >+ (caps & ARCH_CAPS_RSBA) ? " RSBA" : >""); > > /* Compiled-in support which pertains to BTI mitigations. */ > if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) ) >-- >2.1.4 > > >___ >Xen-devel mailing list >Xen-devel@lists.xenproject.org >https://lists.xenproject.org/mailman/listinfo/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/7] doc: correct livepatch.markdown syntax
On Tue, Apr 24, 2018 at 08:44:57AM +0200, Juergen Gross wrote: > "make -C docs all" fails due to incorrect markdown syntax in > livepatch.markdown. Correct it. > > Signed-off-by: Juergen Gross > --- > docs/misc/livepatch.markdown | 589 > --- > 1 file changed, 272 insertions(+), 317 deletions(-) > > diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown > index 54a6b850cb..f3c1320e5a 100644 > --- a/docs/misc/livepatch.markdown > +++ b/docs/misc/livepatch.markdown > @@ -89,33 +89,26 @@ As example we will assume the hypervisor does not have > XSA-132 (see > 4ff3449f0e9d175ceb9551d3f2aecb59273f639d) and we would like to binary patch > the hypervisor with it. The original code looks as so: > > - > - 48 89 e0 mov%rsp,%rax > - 48 25 00 80 ff ff and$0x8000,%rax > - > + 48 89 e0 mov%rsp,%rax > + 48 25 00 80 ff ff and$0x8000,%rax > > while the new patched hypervisor would be: > > - > - 48 c7 45 b8 00 00 00 00 movq $0x0,-0x48(%rbp) > - 48 c7 45 c0 00 00 00 00 movq $0x0,-0x40(%rbp) > - 48 c7 45 c8 00 00 00 00 movq $0x0,-0x38(%rbp) > - 48 89 e0 mov%rsp,%rax > - 48 25 00 80 ff ff and$0x8000,%rax > - > + 48 c7 45 c0 00 00 00 00 movq $0x0,-0x40(%rbp) How come you deleted: 48 c7 45 b8 00 00 00 00 movq $0x0,-0x48(%rbp) ? It is OK, but I am just curious. And either way - if you want to add that back in or not - please add Reviewed-by: Konrad Rzeszutek Wilk Thank you! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/kbdif: Add features to control keyboard and pointer
On Fri, Apr 27, 2018 at 09:58:11AM +0300, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko > > It is now not fully possible to control if and which virtual devices > are created by the frontend, e.g. keyboard and pointer devices > are always created and multi-touch device is created if the s/is/are/ > backend advertises multi-touch support. In some cases this Can you mention under which backend node those devices appear? > behavior is not desirable and better control over the frontend's > configuration is required. > > Add new XenStore feature fields, so it is possible to individually > control set of exposed virtual devices for each guest OS: > - set feature-keyboard to 0 if no keyboard device needs to be created > - set feature-pointer to 0 if no pointer device needs to be created I am thinking that this should be just called 'feature-disable-keyboard' or such. And it being there in the first place would signify '1' by default? > > Keep old behavior by default. > > Signed-off-by: Oleksandr Andrushchenko > --- > xen/include/public/io/kbdif.h | 19 ++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h > index 3ce54e9a44c1..ac92e466fd9c 100644 > --- a/xen/include/public/io/kbdif.h > +++ b/xen/include/public/io/kbdif.h > @@ -49,7 +49,22 @@ > * > * Capable backend advertises supported features by publishing > * corresponding entries in XenStore and puts 1 as the value of the entry. > - * If a feature is not supported then 0 must be set or feature entry omitted. > + * If not otherwise noted if a feature is not supported then 0 must be set > + * or feature entry omitted. Huh? I am not sure what you are saying there. > + * > + * feature-keyboard > + * Values: > + * > + * If no virtual keyboard device to be exposed by the frontend then > + * this must be set to 0. If feature entry omitted or not set its > + * value defaults to 1. Are you saying: "If there is no need to expose a virtual keyboard device then this must be set to 0. By default it is 1 and it is assumed that any frontend that does not probe this flag will assume the value of 1. "? > + * > + * feature-pointer > + * Values: > + * > + * If no virtual pointer device to be exposed by the frontend then > + * this must be set to 0. If feature entry omitted or not set its > + * value defaults to 1. Ditto? > * > * feature-abs-pointer > * Values: > @@ -177,6 +192,8 @@ > > #define XENKBD_DRIVER_NAME "vkbd" > > +#define XENKBD_FIELD_FEAT_KEYBOARD "feature-keyboard" > +#define XENKBD_FIELD_FEAT_POINTER "feature-pointer" How about just call it ' feature-disable-keyboard feature-disable-keyboard > #define XENKBD_FIELD_FEAT_ABS_POINTER "feature-abs-pointer" > #define XENKBD_FIELD_FEAT_MTOUCH "feature-multi-touch" > #define XENKBD_FIELD_REQ_ABS_POINTER "request-abs-pointer" > -- > 2.17.0 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/kbdif: Add features to control keyboard and pointer
On Fri, Apr 27, 2018 at 06:19:35PM +0300, Oleksandr Andrushchenko wrote: > On 04/27/2018 06:11 PM, Konrad Rzeszutek Wilk wrote: > > On Fri, Apr 27, 2018 at 09:58:11AM +0300, Oleksandr Andrushchenko wrote: > > > From: Oleksandr Andrushchenko > > > > > > It is now not fully possible to control if and which virtual devices > > > are created by the frontend, e.g. keyboard and pointer devices > > > are always created and multi-touch device is created if the > > s/is/are/ > why? "and multi-touch *device is* created" > > > > backend advertises multi-touch support. In some cases this > > Can you mention under which backend node those devices appear? > These are created under frontend nodes as these are > to configure individual frontends > > > behavior is not desirable and better control over the frontend's > > > configuration is required. > > > Add new XenStore feature fields, so it is possible to individually > > > control set of exposed virtual devices for each guest OS: > > > - set feature-keyboard to 0 if no keyboard device needs to be created > > > - set feature-pointer to 0 if no pointer device needs to be created > > I am thinking that this should be just called 'feature-disable-keyboard' > > or such. And it being there in the first place would signify '1' by default? > I just tried to be aligned with multi-touch which is > "feature-multi-touch". But if you are ok with > "feature-disable-keyboard/pointer" then I can re-work it > this way. I think disable works nicer - that way you know for sure what the purpose is and there is no confusion about the other features. And the absence of it by default implies you are OK having it be used. > > > Keep old behavior by default. > > > > > > Signed-off-by: Oleksandr Andrushchenko > > > --- > > > xen/include/public/io/kbdif.h | 19 ++- > > > 1 file changed, 18 insertions(+), 1 deletion(-) > > > > > > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h > > > index 3ce54e9a44c1..ac92e466fd9c 100644 > > > --- a/xen/include/public/io/kbdif.h > > > +++ b/xen/include/public/io/kbdif.h > > > @@ -49,7 +49,22 @@ > > >* > > >* Capable backend advertises supported features by publishing > > >* corresponding entries in XenStore and puts 1 as the value of the > > > entry. > > > - * If a feature is not supported then 0 must be set or feature entry > > > omitted. > > > + * If not otherwise noted if a feature is not supported then 0 must be > > > set > > > + * or feature entry omitted. > > Huh? I am not sure what you are saying there. > > > + * > > > + * feature-keyboard > > > + * Values: > > > + * > > > + * If no virtual keyboard device to be exposed by the frontend then > > > + * this must be set to 0. If feature entry omitted or not set its > > > + * value defaults to 1. > > Are you saying: > > "If there is no need to expose a virtual keyboard device then this must be > > set to 0. By default it is 1 and it is assumed that any frontend that does > > not probe this flag will assume the value of 1. "? > yeap ;) > > > + * > > > + * feature-pointer > > > + * Values: > > > + * > > > + * If no virtual pointer device to be exposed by the frontend then > > > + * this must be set to 0. If feature entry omitted or not set its > > > + * value defaults to 1. > > Ditto? > > >* > > >* feature-abs-pointer > > >* Values: > > > @@ -177,6 +192,8 @@ > > > #define XENKBD_DRIVER_NAME "vkbd" > > > +#define XENKBD_FIELD_FEAT_KEYBOARD "feature-keyboard" > > > +#define XENKBD_FIELD_FEAT_POINTER "feature-pointer" > > How about just call it ' > > > > feature-disable-keyboard > > feature-disable-keyboard > See above, I'm fine with that. If we agree on "disable" > semantics I will re-work the patch. > > > #define XENKBD_FIELD_FEAT_ABS_POINTER "feature-abs-pointer" > > > #define XENKBD_FIELD_FEAT_MTOUCH "feature-multi-touch" > > > #define XENKBD_FIELD_REQ_ABS_POINTER "request-abs-pointer" > > > -- > > > 2.17.0 > > > > Thank you, > Oleksandr ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RESEND v1 0/7] Intel Processor Trace virtulization enabling
On Tue, Jan 16, 2018 at 02:12:26AM +0800, Luwei Kang wrote: > Hi All, > > Here is a patch-series which adding Processor Trace enabling in XEN guest. > You can get It's software developer manuals from: > https://software.intel.com/sites/default/files/managed/c5/15/architecture-instruction-set-extensions-programming-reference.pdf > In Chapter 5 INTEL PROCESSOR TRACE: VMX IMPROVEMENTS. Now Chapter 4. > > Introduction: > Intel Processor Trace (Intel PT) is an extension of Intel Architecture that > captures information about software execution using dedicated hardware > facilities that cause only minimal performance perturbation to the software > being traced. Details on the Intel PT infrastructure and trace capabilities > can be found in the Intel 64 and IA-32 Architectures Software Developer’s > Manual, Volume 3C. > > The suite of architecture changes serve to simplify the process of > virtualizing Intel PT for use by a guest software. There are two primary > elements to this new architecture support for VMX support improvements made > for Intel PT. > 1. Addition of a new guest IA32_RTIT_CTL value field to the VMCS. > — This serves to speed and simplify the process of disabling trace on VM > exit, and restoring it on VM entry. > 2. Enabling use of EPT to redirect PT output. > — This enables the VMM to elect to virtualize the PT output buffer using > EPT. In this mode, the CPU will treat PT output addresses as Guest Physical > Addresses (GPAs) and translate them using EPT. This means that Intel PT > output reads (of the ToPA table) and writes (of trace output) can cause EPT > violations, and other output events. > How does one test this functionality in Linux? As in does 'perf' take advantage of the Processor Trace functionality? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RESEND v1 3/7] x86: add intel proecessor trace support for cpuid
On Tue, Jan 16, 2018 at 02:12:29AM +0800, Luwei Kang wrote: > This patch add Intel processor trace support > for cpuid handling. The 0x14 usage screams of wanting an #define. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] xen/kbdif: Add features to disable keyboard and pointer
On Wed, May 02, 2018 at 10:16:08AM +0300, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko > > It is now not fully possible to control if and which virtual devices > are created by the frontend, e.g. keyboard and pointer devices > are always created and multi-touch device is created if the > backend advertises multi-touch support. In some cases this > behavior is not desirable and better control over the frontend's > configuration is required. > > Add new XenStore feature fields, so it is possible to individually > control set of exposed virtual devices for each guest OS: > - set feature-disable-keyboard to 1 if no keyboard device needs >to be created > - set feature-disable-pointer to 1 if no pointer device needs >to be created > > Keep old behavior by default. > > Signed-off-by: Oleksandr Andrushchenko Reviewed-by: Konrad Rzeszutek Wilk Thank you! > --- > xen/include/public/io/kbdif.h | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h > index 3ce54e9a44c1..9a0648bdced9 100644 > --- a/xen/include/public/io/kbdif.h > +++ b/xen/include/public/io/kbdif.h > @@ -51,6 +51,18 @@ > * corresponding entries in XenStore and puts 1 as the value of the entry. > * If a feature is not supported then 0 must be set or feature entry omitted. > * > + * feature-disable-keyboard > + * Values: > + * > + * If there is no need to expose a virtual keyboard device by the > + * frontend then this must be set to 1. > + * > + * feature-disable-pointer > + * Values: > + * > + * If there is no need to expose a virtual pointer device by the > + * frontend then this must be set to 1. > + * > * feature-abs-pointer > * Values: > * > @@ -177,6 +189,8 @@ > > #define XENKBD_DRIVER_NAME "vkbd" > > +#define XENKBD_FIELD_FEAT_DSBL_KEYBRD "feature-disable-keyboard" > +#define XENKBD_FIELD_FEAT_DSBL_POINTER "feature-disable-pointer" > #define XENKBD_FIELD_FEAT_ABS_POINTER "feature-abs-pointer" > #define XENKBD_FIELD_FEAT_MTOUCH "feature-multi-touch" > #define XENKBD_FIELD_REQ_ABS_POINTER "request-abs-pointer" > -- > 2.17.0 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 1/2] xen/kbdif: Add string constants for raw pointer
On Wed, May 02, 2018 at 05:49:18PM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko
>
> Add missing string constants for {feature|request}-raw-pointer
> to align with the rest of the interface file.
>
> Fixes 7868654ff7fe ("kbdif: Define "feature-raw-pointer" and
> "request-raw-pointer")
>
> Signed-off-by: Oleksandr Andrushchenko
Reviewed-by: Konrad Rzeszutek Wilk
Thank you!
> ---
> xen/include/public/io/kbdif.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h
> index 3ce54e9a44c1..daf4bc2063c9 100644
> --- a/xen/include/public/io/kbdif.h
> +++ b/xen/include/public/io/kbdif.h
> @@ -178,8 +178,10 @@
> #define XENKBD_DRIVER_NAME "vkbd"
>
> #define XENKBD_FIELD_FEAT_ABS_POINTER "feature-abs-pointer"
> +#define XENKBD_FIELD_FEAT_RAW_POINTER "feature-raw-pointer"
> #define XENKBD_FIELD_FEAT_MTOUCH "feature-multi-touch"
> #define XENKBD_FIELD_REQ_ABS_POINTER "request-abs-pointer"
> +#define XENKBD_FIELD_REQ_RAW_POINTER "request-raw-pointer"
> #define XENKBD_FIELD_REQ_MTOUCH"request-multi-touch"
> #define XENKBD_FIELD_RING_GREF "page-gref"
> #define XENKBD_FIELD_EVT_CHANNEL "event-channel"
> --
> 2.17.0
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 1/2] xen/kbdif: Add string constants for raw pointer
On Fri, May 11, 2018 at 09:37:57AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, May 02, 2018 at 05:49:18PM +0300, Oleksandr Andrushchenko wrote:
> > From: Oleksandr Andrushchenko
> >
> > Add missing string constants for {feature|request}-raw-pointer
> > to align with the rest of the interface file.
> >
> > Fixes 7868654ff7fe ("kbdif: Define "feature-raw-pointer" and
> > "request-raw-pointer")
> >
> > Signed-off-by: Oleksandr Andrushchenko
>
>
> Reviewed-by: Konrad Rzeszutek Wilk
Juergen, you OK with an release-ack?
>
> Thank you!
> > ---
> > xen/include/public/io/kbdif.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h
> > index 3ce54e9a44c1..daf4bc2063c9 100644
> > --- a/xen/include/public/io/kbdif.h
> > +++ b/xen/include/public/io/kbdif.h
> > @@ -178,8 +178,10 @@
> > #define XENKBD_DRIVER_NAME "vkbd"
> >
> > #define XENKBD_FIELD_FEAT_ABS_POINTER "feature-abs-pointer"
> > +#define XENKBD_FIELD_FEAT_RAW_POINTER "feature-raw-pointer"
> > #define XENKBD_FIELD_FEAT_MTOUCH "feature-multi-touch"
> > #define XENKBD_FIELD_REQ_ABS_POINTER "request-abs-pointer"
> > +#define XENKBD_FIELD_REQ_RAW_POINTER "request-raw-pointer"
> > #define XENKBD_FIELD_REQ_MTOUCH"request-multi-touch"
> > #define XENKBD_FIELD_RING_GREF "page-gref"
> > #define XENKBD_FIELD_EVT_CHANNEL "event-channel"
> > --
> > 2.17.0
> >
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 01/10] x86/spec_ctrl: Read MSR_ARCH_CAPABILITIES only once
On Fri, May 11, 2018 at 11:38:05AM +0100, Andrew Cooper wrote:
> Make it available from the beginning of init_speculation_mitigations(), and
> pass it into appropriate functions. Fix an RSBA typo while moving the
> affected comment.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Konrad Rzeszutek Wilk
Thank you!
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Juergen Gross
> ---
> xen/arch/x86/spec_ctrl.c | 34 ++
> 1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> index 037e84d..4ab0f50 100644
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -97,18 +97,15 @@ static int __init parse_bti(const char *s)
> }
> custom_param("bti", parse_bti);
>
> -static void __init print_details(enum ind_thunk thunk)
> +static void __init print_details(enum ind_thunk thunk, uint64_t caps)
> {
> unsigned int _7d0 = 0, e8b = 0, tmp;
> -uint64_t caps = 0;
>
> /* Collect diagnostics about available mitigations. */
> if ( boot_cpu_data.cpuid_level >= 7 )
> cpuid_count(7, 0, &tmp, &tmp, &tmp, &_7d0);
> if ( boot_cpu_data.extended_cpuid_level >= 0x8008 )
> cpuid(0x8008, &tmp, &e8b, &tmp, &tmp);
> -if ( _7d0 & cpufeat_mask(X86_FEATURE_ARCH_CAPS) )
> -rdmsrl(MSR_ARCH_CAPABILITIES, caps);
>
> printk(XENLOG_DEBUG "Speculative mitigation facilities:\n");
>
> @@ -142,7 +139,7 @@ static void __init print_details(enum ind_thunk thunk)
> }
>
> /* Calculate whether Retpoline is known-safe on this CPU. */
> -static bool __init retpoline_safe(void)
> +static bool __init retpoline_safe(uint64_t caps)
> {
> unsigned int ucode_rev = this_cpu(ucode_cpu_info).cpu_sig.rev;
>
> @@ -153,19 +150,12 @@ static bool __init retpoline_safe(void)
> boot_cpu_data.x86 != 6 )
> return false;
>
> -if ( boot_cpu_has(X86_FEATURE_ARCH_CAPS) )
> -{
> -uint64_t caps;
> -
> -rdmsrl(MSR_ARCH_CAPABILITIES, caps);
> -
> -/*
> - * RBSA may be set by a hypervisor to indicate that we may move to a
> - * processor which isn't retpoline-safe.
> - */
> -if ( caps & ARCH_CAPS_RSBA )
> -return false;
> -}
> +/*
> + * RSBA may be set by a hypervisor to indicate that we may move to a
> + * processor which isn't retpoline-safe.
> + */
> +if ( caps & ARCH_CAPS_RSBA )
> +return false;
>
> switch ( boot_cpu_data.x86_model )
> {
> @@ -299,6 +289,10 @@ void __init init_speculation_mitigations(void)
> {
> enum ind_thunk thunk = THUNK_DEFAULT;
> bool ibrs = false;
> +uint64_t caps = 0;
> +
> +if ( boot_cpu_has(X86_FEATURE_ARCH_CAPS) )
> +rdmsrl(MSR_ARCH_CAPABILITIES, caps);
>
> /*
> * Has the user specified any custom BTI mitigations? If so, follow
> their
> @@ -327,7 +321,7 @@ void __init init_speculation_mitigations(void)
> * On Intel hardware, we'd like to use retpoline in preference to
> * IBRS, but only if it is safe on this hardware.
> */
> -else if ( retpoline_safe() )
> +else if ( retpoline_safe(caps) )
> thunk = THUNK_RETPOLINE;
> else if ( boot_cpu_has(X86_FEATURE_IBRSB) )
> ibrs = true;
> @@ -418,7 +412,7 @@ void __init init_speculation_mitigations(void)
> else
> setup_clear_cpu_cap(X86_FEATURE_NO_XPTI);
>
> -print_details(thunk);
> +print_details(thunk, caps);
> }
>
> static void __init __maybe_unused build_assertions(void)
> --
> 2.1.4
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/2] doc: correct livepatch.markdown syntax
On Tue, May 08, 2018 at 11:51:47AM +0100, George Dunlap wrote: > On 05/08/2018 07:47 AM, Juergen Gross wrote: > > "make -C docs all" fails due to incorrect markdown syntax in > > livepatch.markdown. Correct it. > > > > Signed-off-by: Juergen Gross > > Reviewed-by: Konrad Rzeszutek Wilk > > Git complains of trailing whitespace: > > Importing patch "doc-correct-livepatch-markdown" ... :69: > trailing whitespace. > > :72: trailing whitespace. > > :98: trailing whitespace. > > :232: trailing whitespace. > > :238: trailing whitespace. That is on purpose. That is you need those two spaces at the end to force it to stay in 'code' mode. > > Checking patch docs/misc/livepatch.markdown... > Applied patch docs/misc/livepatch.markdown cleanly. > warning: squelched 13 whitespace errors > warning: 18 lines add whitespace errors. > > > > --- > > docs/misc/livepatch.markdown | 590 > > --- > > 1 file changed, 273 insertions(+), 317 deletions(-) > > > > diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown > > index 54a6b850cb..a4de44472a 100644 > > --- a/docs/misc/livepatch.markdown > > +++ b/docs/misc/livepatch.markdown > > @@ -89,33 +89,27 @@ As example we will assume the hypervisor does not have > > XSA-132 (see > > 4ff3449f0e9d175ceb9551d3f2aecb59273f639d) and we would like to binary patch > > the hypervisor with it. The original code looks as so: > > > > - > > - 48 89 e0 mov%rsp,%rax > > - 48 25 00 80 ff ff and$0x8000,%rax > > - > > + 48 89 e0 mov%rsp,%rax > > + 48 25 00 80 ff ff and$0x8000,%rax > > > > while the new patched hypervisor would be: > > > > - > > - 48 c7 45 b8 00 00 00 00 movq $0x0,-0x48(%rbp) > > - 48 c7 45 c0 00 00 00 00 movq $0x0,-0x40(%rbp) > > - 48 c7 45 c8 00 00 00 00 movq $0x0,-0x38(%rbp) > > - 48 89 e0 mov%rsp,%rax > > - 48 25 00 80 ff ff and$0x8000,%rax > > - > > + 48 c7 45 b8 00 00 00 00 movq $0x0,-0x48(%rbp) > > + 48 c7 45 c0 00 00 00 00 movq $0x0,-0x40(%rbp) > > + 48 c7 45 c8 00 00 00 00 movq $0x0,-0x38(%rbp) > > + 48 89 e0 mov%rsp,%rax > > + 48 25 00 80 ff ff and$0x8000,%rax > > > > -This is inside the arch_do_domctl. This new change adds 21 extra > > +This is inside the arch\_do\_domctl. This new change adds 21 extra > > It seems like nearly all of these would be better served by making them > code blocks ( `arch_do_domctl`). It is: > * easier to read in text format (one of the main points of markdown), > * fewer characters to type, > * looks better rendered into html or pdf, and > * doesn't require < > tags for < and >. > > -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/1] x86/PVHv2: Add memory map pointer to hvm_start_info struct
On Fri, Mar 02, 2018 at 12:54:29PM -0800, Maran Wilson wrote:
> The start info structure that is defined as part of the x86/HVM direct boot
> ABI and used for starting Xen PVH guests would be more versatile if it also
> included a way to pass information about the memory map to the guest. This
> would allow KVM guests to share the same entry point.
Would it be better if there was an tag/length as well? And maybe more dynamic
so that if you want to add more structures you can identify them tags?
Like what Multiboot2 has?
>
> Signed-off-by: Maran Wilson
> ---
> xen/include/public/arch-x86/hvm/start_info.h | 51
> +++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/public/arch-x86/hvm/start_info.h
> b/xen/include/public/arch-x86/hvm/start_info.h
> index 6484159..ae8dac8 100644
> --- a/xen/include/public/arch-x86/hvm/start_info.h
> +++ b/xen/include/public/arch-x86/hvm/start_info.h
> @@ -33,8 +33,9 @@
> *| magic | Contains the magic value XEN_HVM_START_MAGIC_VALUE
> *|| ("xEn3" with the 0x80 bit of the "E" set).
> * 4 ++
> - *| version| Version of this structure. Current version is 0. New
> + *| version| Version of this structure. Current version is 1. New
> *|| versions are guaranteed to be backwards-compatible.
> + *|| For PV guests only 0 allowed, for PVH 0 or 1
> allowed.
> * 8 ++
> *| flags | SIF_xxx flags.
> * 12 ++
> @@ -48,6 +49,15 @@
> * 32 ++
> *| rsdp_paddr | Physical address of the RSDP ACPI data structure.
> * 40 ++
> + *| memmap_paddr | Physical address of the (optional) memory map. Only
> + *|| present in version 1 and newer of the structure.
> + * 48 ++
> + *| memmap_entries | Number of entries in the memory map table. Only
> + *|| present in version 1 and newer of the structure.
> + *|| Zero if there is no memory map being provided.
> + * 52 ++
> + *| reserved | Version 1 and newer only.
> + * 56 ++
> *
> * The layout of each entry in the module structure is the following:
> *
> @@ -62,10 +72,34 @@
> *| reserved |
> * 32 ++
> *
> + * The layout of each entry in the memory map table is as follows:
> + *
> + * 0 ++
> + *| addr | Base address
> + * 8 ++
> + *| size | Size of mapping in bytes
> + * 16 ++
> + *| type | Type of mapping as defined between the hypervisor
> + *|| and guest it's starting. E820_TYPE_xxx, for example.
> + * 20 +|
> + *| reserved |
> + * 24 ++
> + *
> * The address and sizes are always a 64bit little endian unsigned integer.
> *
> * NB: Xen on x86 will always try to place all the data below the 4GiB
> * boundary.
> + *
> + * Version numbers of the hvm_start_info structure have evolved like this:
> + *
> + * Version 0:
> + *
> + * Version 1:Added the memmap_paddr/memmap_entries fields (plus 4
> bytes of
> + * padding) to the end of the hvm_start_info struct. These new
> + * fields can be used to pass a memory map to the guest. The
> + * memory map is optional and so guests that understand version 1
> + * of the structure must check that memmap_entries is non-zero
> + * before trying to read the memory map.
> */
> #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
>
> @@ -86,6 +120,14 @@ struct hvm_start_info {
> uint64_t cmdline_paddr; /* Physical address of the command line.
> */
> uint64_t rsdp_paddr;/* Physical address of the RSDP ACPI data
> */
> /* structure.
> */
> +uint64_t memmap_paddr; /* Physical address of an array of */
> + /* hvm_memmap_table_entry. Only present in */
> + /* version 1 and newer of the structure */
> +uint32_t memmap_entries; /* Number of entries in the memmap table.*/
> + /* Only present in version 1 and newer of*/
> + /* the structure. Value will be zero if */
> + /* there is no memory map being provided.*/
> +uint32_t reserved;
> };
>
> struct hvm_modlist_entry {
> @@ -95,4 +137,11 @@ struct hvm_modlist_entry {
> uint64_t reserved;
> };
>
> +struct hvm_memmap_table_entry {
> +uint64_t addr; /* Base address of the memory region */
> +uint64_t size; /* Size of the memory region in bytes*/
> +uint32_t type; /* Mapping type
Re: [Xen-devel] [PATCH LP-BUILD-TOOLS] Allow patching files compiled multiple times
On Mon, Mar 05, 2018 at 10:32:51AM +, Ross Lagerwall wrote:
> On 02/23/2018 05:08 PM, Ross Lagerwall wrote:
> > gas prior to binutils commit fbdf9406b0 (appears in 2.27) outputs symbol
> > table entries resulting from .file in reverse order. If we get two
> > consecutive file symbols, prefer the first one if that names an object
> > file or has a directory component (to cover multiply compiled files).
> >
> > This is the same workaround that was applied in Xen commit d37d63d4b548
> > ("symbols: prefix static symbols with their source file names") for
> > Xen's internal symbol table.
> >
> > This fixes building a livepatch for XSA-243.
> > ---
> > lookup.c | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/lookup.c b/lookup.c
> > index 39125c6..645b91a 100644
> > --- a/lookup.c
> > +++ b/lookup.c
> > @@ -149,16 +149,34 @@ int lookup_local_symbol(struct lookup_table *table,
> > char *name, char *hint,
> > struct symbol *sym, *match = NULL;
> > int i;
> > char *curfile = NULL;
> > + enum { other, multi_source } last_type = other;
> > memset(result, 0, sizeof(*result));
> > for_each_symbol(i, sym, table) {
> > if (sym->type == STT_FILE) {
> > + const char *ext = strrchr(sym->name, '.');
> > + int multi = strchr(sym->name, '/') ||
> > + (ext && ext[1] == 'o');
> > +
> > + /*
> > +* gas prior to binutils commit fbdf9406b0 (appears in
> > +* 2.27) outputs symbol table entries resulting from
> > +* .file in reverse order. If we get two consecutive
> > +* file symbols, prefer the first one if that names an
> > +* object file or has a directory component (to cover
> > +* multiply compiled files).
> > +*/
> > + if (last_type == multi_source)
> > + continue;
> > +
> > if (!strcmp(sym->name, hint)) {
> > curfile = sym->name;
> > + last_type = multi ? multi_source : other;
> > continue; /* begin hint file symbols */
Perhaps add some logging as well?
> > } else if (curfile)
> > curfile = NULL; /* end hint file symbols */
> > }
> > + last_type = other;
> > if (!curfile)
> > continue;
> > if (sym->bind == STB_LOCAL && !strcmp(sym->name, name)) {
> >
>
> Ping, Konrad?
This looks familiar, but how come it has no SoB?
>
> --
> Ross Lagerwall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] Xen Virtio Drivers
On Wed, Mar 07, 2018 at 04:13:47PM +0200, Volo M. wrote:
> Hi Devs,
>
> Could you please help me to identify right way to start using Virtio
> drivers for Xen guests
> as described here https://wiki.xen.org/wiki/QEMU_Upstream .
> I've managed to start my Windows 2008 VM with Virtio network drivers by
> using 'model='virtio-net'' option as described above and it works fine (but
> only for network interfaces).
>
> The problem is I still can't identify how to make Xen using virtio-blk or
> virtio-scsi backend for 'qemu-xen' qemu-model as mentioned here
> https://en.wikibooks.org/wiki/QEMU/Devices/Virtio which should be
> compatible with 'qemu-xen' qemu upstream model.
>
> I can't see any mention about virtio disk types in docs:
> https://xenbits.xen.org/docs/4.8-testing/misc/xl-disk-configuration.txt .
> Also I've done very common check through Xen source code ('*/tools/libxl/*'
> area) while looking how to apply virtio disk drivers for our Xen guests.
>
> Can you please clarify if it's been ever implemented. Do you have any ETA
> when it's going to be implemented...etc? Or probably you could give me any
> advice how to make it working?
IT all works with HVM guests. You do have to tweak the Xen build of qemu
to build with KVM enabled (oddly enough the Virtio drivers depend on that
right now).
>
> Thanks a lot in advance.
> ___
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] x86: Fix APIC MSR constant names
On Wed, Mar 07, 2018 at 06:58:34PM +, Andrew Cooper wrote: > We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are > synonymous from a naming point of view, but refer to very different things. > > Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and > visually separate the register function from the generic APIC name. For the > case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge > that there are 0x3ff MSRs architecturally reserved for x2APIC functionality. > > For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR > itself, but drop the MSR prefix from the other constants to shorten the names. > In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious > from the context. > > No functional change (the combined binary is identical). > > Signed-off-by: Andrew Cooper Reviewed-by: Konrad Rzeszutek Wilk Thank you! ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH LINUX] Fixes to xen-blkfront.
Hi! We found this tiny bug when we were unplugging and re-plugging different block-devices to a guest and found a bug in there resulting in the multi-queue parameter being lost. This patch fixes it. I meant to post this in December but got side-tracked with Spectre_v2 work. drivers/block/xen-blkfront.c | 17 - 1 file changed, 8 insertions(+), 9 deletions(-) Bhavesh Davda (1): xen-blkfront: move negotiate_mq to cover all cases of new VBDs ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/5] x86/hvm: Handle x2apic MSRs via the new guest_{rd, wr}msr() infrastructure
On Wed, Mar 07, 2018 at 06:58:35PM +, Andrew Cooper wrote:
> Dispatch from the guest_{rd,wr}msr() functions. The read side should be safe
> outside of current context, but the write side is definitely not. As the
> toolstack has no legitimate reason to access the APIC registers via this
> interface (not least because whether they are accessible at all depends on
> guest settings), unilaterally reject access attempts outside of current
> context.
>
> Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
> to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE. The previous
> callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
> interfere with the fallback to legacy MSR handling.
>
> While altering guest_rdmsr_x2apic() make a couple of minor improvements.
> Reformat the initialiser for readable[] so it indents in a more natural way,
> and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
> path.
>
> Observant people might notice that we now don't let PV guests read the x2apic
> MSRs. They should never have been able to in the first place.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Konrad Rzeszutek Wilk
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen-blkfront: move negotiate_mq to cover all cases of new VBDs
From: Bhavesh Davda
negotiate_mq should happen in all cases of a new VBD being discovered by
xen-blkfront, whether called through _probe() or a hot-attached new VBD
from dom-0 via xenstore. Otherwise, hot-attached new VBDs are left
configured without multi-queue.
Signed-off-by: Bhavesh Davda
Reviewed-by: Konrad Rzeszutek Wilk
Signed-off-by: Konrad Rzeszutek Wilk
---
drivers/block/xen-blkfront.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 891265acb10e..7d23225f79ed 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -262,6 +262,7 @@ static DEFINE_SPINLOCK(minor_lock);
static int blkfront_setup_indirect(struct blkfront_ring_info *rinfo);
static void blkfront_gather_backend_features(struct blkfront_info *info);
+static int negotiate_mq(struct blkfront_info *info);
static int get_id_from_freelist(struct blkfront_ring_info *rinfo)
{
@@ -1774,11 +1775,18 @@ static int talk_to_blkback(struct xenbus_device *dev,
unsigned int i, max_page_order;
unsigned int ring_page_order;
+ if (!info)
+ return -ENODEV;
+
max_page_order = xenbus_read_unsigned(info->xbdev->otherend,
"max-ring-page-order", 0);
ring_page_order = min(xen_blkif_max_ring_order, max_page_order);
info->nr_ring_pages = 1 << ring_page_order;
+ err = negotiate_mq(info);
+ if (err)
+ goto destroy_blkring;
+
for (i = 0; i < info->nr_rings; i++) {
struct blkfront_ring_info *rinfo = &info->rinfo[i];
@@ -1978,11 +1986,6 @@ static int blkfront_probe(struct xenbus_device *dev,
}
info->xbdev = dev;
- err = negotiate_mq(info);
- if (err) {
- kfree(info);
- return err;
- }
mutex_init(&info->mutex);
info->vdevice = vdevice;
@@ -2099,10 +2102,6 @@ static int blkfront_resume(struct xenbus_device *dev)
blkif_free(info, info->connected == BLKIF_STATE_CONNECTED);
- err = negotiate_mq(info);
- if (err)
- return err;
-
err = talk_to_blkback(dev, info);
if (!err)
blk_mq_update_nr_hw_queues(&info->tag_set, info->nr_rings);
--
2.13.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
