Re: [PATCH] hw/display/virtio-vga: made vga memory size configurable
On Mon, Mar 15, 2021 at 4:24 PM Gerd Hoffmann wrote: > > On Mon, Mar 15, 2021 at 12:29:16PM +0100, Vitaly Chipounov wrote: > > On Mon, Mar 15, 2021 at 8:21 AM Gerd Hoffmann wrote: > > > > > > If your guest has no virtio driver use stdvga instead of running > > > virtio-vga permanently in vga compatibility mode. > > > > I tried -device VGA,vgamem_mb=32. I did not see any resolution above > > 1080p on a Windows 10 guest. > > Try "-device VGA,vgamem_mb=32,edid=off". Windows seems to not like our > edid block for some reason. > This worked, thanks for the tip. Regarding the patch, I can resubmit it with an amended commit message if people think it's still better to have a configurable memory size here. Vitaly
Re: [PATCH] hw/display/virtio-vga: made vga memory size configurable
Yes, it's a typo in the commit message, sorry. Vitaly On 3/14/21 1:45 PM, BALATON Zoltan wrote: On Sun, 14 Mar 2021, vit...@cyberhaven.com wrote: From: Vitaly Chipounov This enables higher resolutions. The default is still 8MB for backwards compatibility with existing snapshots. The property name "vgamem_fb" is similar to that of the other Isn't that vgamem_mb? Code has that so it's just a typo in commit message. Regards, BALATON Zoltan graphic adapters. seabios/vgasrc/svgamodes.c needs to be updated as well. For example, adding the following line would expose a 3840x2160 resolution to the guest. { 0x199, { MM_DIRECT, 3840, 2160, 32, 8, 16, SEG_GRAPH } }, Signed-off-by: Vitaly Chipounov --- hw/display/virtio-vga.c | 3 ++- hw/virtio/virtio-pci.h | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c index d3c6404061..657fafc48f 100644 --- a/hw/display/virtio-vga.c +++ b/hw/display/virtio-vga.c @@ -118,7 +118,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp) int i; /* init vga compat bits */ - vga->vram_size_mb = 8; + vga->vram_size_mb = vpci_dev->vgamem_size_mb; vga_common_init(vga, OBJECT(vpci_dev)); vga_init(vga, OBJECT(vpci_dev), pci_address_space(&vpci_dev->pci_dev), pci_address_space_io(&vpci_dev->pci_dev), true); @@ -204,6 +204,7 @@ static void virtio_vga_set_big_endian_fb(Object *obj, bool value, Error **errp) static Property virtio_vga_base_properties[] = { DEFINE_VIRTIO_GPU_PCI_PROPERTIES(VirtIOPCIProxy), + DEFINE_PROP_UINT32("vgamem_mb", VirtIOPCIProxy, vgamem_size_mb, 8), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h index d7d5d403a9..8684311a8d 100644 --- a/hw/virtio/virtio-pci.h +++ b/hw/virtio/virtio-pci.h @@ -151,6 +151,8 @@ struct VirtIOPCIProxy { VirtIOIRQFD *vector_irqfd; int nvqs_with_notifiers; VirtioBusState bus; + + uint32_t vgamem_size_mb; }; static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
Re: [PATCH] hw/display/virtio-vga: made vga memory size configurable
On Mon, Mar 15, 2021 at 8:21 AM Gerd Hoffmann wrote: > > On Sun, Mar 14, 2021 at 01:23:14PM +0100, vit...@cyberhaven.com wrote: > > From: Vitaly Chipounov > > > > This enables higher resolutions. > > No. virtio-vga supports higher resolutions just fine once the guest > driver is loaded. The video memory is used at boot only, before the > guest driver is loaded, and 8MB just for a boot display is more than > generous. > > If your guest has no virtio driver use stdvga instead of running > virtio-vga permanently in vga compatibility mode. I tried -device VGA,vgamem_mb=32. I did not see any resolution above 1080p on a Windows 10 guest. virtio-vga has many more resolutions available, it was just missing 4k. I have the virtio-win-0.1.190 driver pack installed. I don't use qxl, because it makes the Windows GUI sluggish for me. I don't have problems with VGA or virtio-vga. Best regards, Vitaly
[Qemu-devel] [PATCH] x86: Fixed incorrect segment base address addition
An instruction with address and segment size override triggers the bug. inc dword ptr gs:260h[ebx*4] gets incorrectly translated to: (uint32_t)(gs.base + ebx * 4 + 0x260) instead of gs.base + (uint32_t)(ebx * 4 + 0x260) Signed-off-by: Vitaly Chipounov --- target-i386/translate.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/target-i386/translate.c b/target-i386/translate.c index a902f4a..9ca7375 100644 --- a/target-i386/translate.c +++ b/target-i386/translate.c @@ -459,10 +459,10 @@ static inline void gen_op_movl_A0_seg(int reg) static inline void gen_op_addl_A0_seg(int reg) { tcg_gen_ld_tl(cpu_tmp0, cpu_env, offsetof(CPUX86State, segs[reg].base)); -tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); #ifdef TARGET_X86_64 tcg_gen_andi_tl(cpu_A0, cpu_A0, 0x); #endif +tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); } #ifdef TARGET_X86_64 -- 1.7.4.1
Re: [Qemu-devel] [PATCH] x86: Fixed incorrect segment base address addition
Max, On 02.07.2012 17:18, Max Filippov wrote: > On Mon, Jul 2, 2012 at 2:29 PM, Vitaly Chipounov > wrote: >> An instruction with address and segment size override triggers the bug. >> inc dword ptr gs:260h[ebx*4] gets incorrectly translated to: >> (uint32_t)(gs.base + ebx * 4 + 0x260) >> instead of >> gs.base + (uint32_t)(ebx * 4 + 0x260) > Do I understand it right that this fixes address calculation for > 64-bit mode but breaks it for compatibility mode? You are right, it indeed breaks compatibility mode. Thanks for the reference from the Intel manual. I will send an updated patch. Vitaly >> Signed-off-by: Vitaly Chipounov >> --- >> target-i386/translate.c |2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/target-i386/translate.c b/target-i386/translate.c >> index a902f4a..9ca7375 100644 >> --- a/target-i386/translate.c >> +++ b/target-i386/translate.c >> @@ -459,10 +459,10 @@ static inline void gen_op_movl_A0_seg(int reg) >> static inline void gen_op_addl_A0_seg(int reg) >> { >> tcg_gen_ld_tl(cpu_tmp0, cpu_env, offsetof(CPUX86State, segs[reg].base)); >> -tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); >> #ifdef TARGET_X86_64 >> tcg_gen_andi_tl(cpu_A0, cpu_A0, 0x); >> #endif >> +tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); >> } >> >> #ifdef TARGET_X86_64 >> -- >> 1.7.4.1 >> >> > >
[Qemu-devel] [PATCH v2] x86: Fixed incorrect segment base address addition in 64-bits mode
According to the Intel manual "Intel® 64 and IA-32 Architectures Software Developer’s Manual Volume 3", "3.4.4 Segment Loading Instructions in IA-32e Mode": "When in compatibility mode, FS and GS overrides operate as defined by 32-bit mode behavior regardless of the value loaded into the upper 32 linear-address bits of the hidden descriptor register base field. Compatibility mode ignores the upper 32 bits when calculating an effective address." However, the code misses the 64-bit mode case, where an instruction with address and segment size override would be translated incorrectly. For example, inc dword ptr gs:260h[ebx*4] gets incorrectly translated to: (uint32_t)(gs.base + ebx * 4 + 0x260) instead of gs.base + (uint32_t)(ebx * 4 + 0x260) Signed-off-by: Vitaly Chipounov --- target-i386/translate.c | 43 +-- 1 files changed, 25 insertions(+), 18 deletions(-) diff --git a/target-i386/translate.c b/target-i386/translate.c index a902f4a..dfec735 100644 --- a/target-i386/translate.c +++ b/target-i386/translate.c @@ -456,12 +456,19 @@ static inline void gen_op_movl_A0_seg(int reg) tcg_gen_ld32u_tl(cpu_A0, cpu_env, offsetof(CPUX86State, segs[reg].base) + REG_L_OFFSET); } -static inline void gen_op_addl_A0_seg(int reg) +static inline void gen_op_addl_A0_seg(DisasContext *s, int reg) { tcg_gen_ld_tl(cpu_tmp0, cpu_env, offsetof(CPUX86State, segs[reg].base)); -tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); #ifdef TARGET_X86_64 -tcg_gen_andi_tl(cpu_A0, cpu_A0, 0x); +if (CODE64(s)) { +tcg_gen_andi_tl(cpu_A0, cpu_A0, 0x); +tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); +} else { +tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); +tcg_gen_andi_tl(cpu_A0, cpu_A0, 0x); +} +#else +tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0); #endif } @@ -617,7 +624,7 @@ static inline void gen_string_movl_A0_ESI(DisasContext *s) override = R_DS; gen_op_movl_A0_reg(R_ESI); gen_op_andl_A0_(); -gen_op_addl_A0_seg(override); +gen_op_addl_A0_seg(s, override); } } @@ -638,7 +645,7 @@ static inline void gen_string_movl_A0_EDI(DisasContext *s) } else { gen_op_movl_A0_reg(R_EDI); gen_op_andl_A0_(); -gen_op_addl_A0_seg(R_ES); +gen_op_addl_A0_seg(s, R_ES); } } @@ -2063,7 +2070,7 @@ static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_ } else #endif { -gen_op_addl_A0_seg(override); +gen_op_addl_A0_seg(s, override); } } } else { @@ -2130,7 +2137,7 @@ static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_ else override = R_DS; } -gen_op_addl_A0_seg(override); +gen_op_addl_A0_seg(s, override); } } @@ -2207,7 +2214,7 @@ static void gen_add_A0_ds_seg(DisasContext *s) } else #endif { -gen_op_addl_A0_seg(override); +gen_op_addl_A0_seg(s, override); } } } @@ -2460,12 +2467,12 @@ static void gen_push_T0(DisasContext *s) if (s->ss32) { if (s->addseg) { tcg_gen_mov_tl(cpu_T[1], cpu_A0); -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } } else { gen_op_andl_A0_(); tcg_gen_mov_tl(cpu_T[1], cpu_A0); -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } gen_op_st_T0_A0(s->dflag + 1 + s->mem_index); if (s->ss32 && !s->addseg) @@ -2500,11 +2507,11 @@ static void gen_push_T1(DisasContext *s) gen_op_addl_A0_im(-4); if (s->ss32) { if (s->addseg) { -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } } else { gen_op_andl_A0_(); -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } gen_op_st_T1_A0(s->dflag + 1 + s->mem_index); @@ -2528,10 +2535,10 @@ static void gen_pop_T0(DisasContext *s) gen_op_movl_A0_reg(R_ESP); if (s->ss32) { if (s->addseg) -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } else { gen_op_andl_A0_(); -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } gen_op_ld_T0_A0(s->dflag + 1 + s->mem_index); } @@ -2556,7 +2563,7 @@ static void gen_stack_A0(DisasContext *s) gen_op_andl_A0_(); tcg_gen_mov_tl(cpu_T[1], cpu_A0); if (s->addseg) -gen_op_addl_A0_seg(R_SS); +gen_op_addl_A0_seg(s, R_SS); } /* NOTE: wrap around in 16 b
[Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
A socket may still have references to it in various queues at the time it is freed, causing memory corruptions. Signed-off-by: Vitaly Chipounov --- slirp/socket.c | 29 + 1 file changed, 29 insertions(+) diff --git a/slirp/socket.c b/slirp/socket.c index 77b0c98..8a7adc8 100644 --- a/slirp/socket.c +++ b/slirp/socket.c @@ -55,6 +55,33 @@ socreate(Slirp *slirp) return(so); } +/** + * It may happen that a socket is still referenced in various + * mbufs at the time it is freed. Clear all references to the + * socket here. + */ +static void soremovefromqueues(struct socket *so) +{ +if (!so->so_queued) { +return; +} + +Slirp *slirp = so->slirp; +struct mbuf *ifm; + +for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = ifm->ifq_next) { +if (ifm->ifq_so == so) { +ifm->ifq_so = NULL; +} +} + +for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = ifm->ifq_next) { +if (ifm->ifq_so == so) { +ifm->ifq_so = NULL; +} +} +} + /* * remque and free a socket, clobber cache */ @@ -79,6 +106,8 @@ sofree(struct socket *so) if(so->so_next && so->so_prev) remque(so); /* crashes if so is not in a queue */ + soremovefromqueues(so); + free(so); } -- 1.7.10
Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket
Hi, On 21.02.2013 15:33, Jan Kiszka wrote: > On 2013-02-15 12:00, Vitaly Chipounov wrote: >> A socket may still have references to it in various queues >> at the time it is freed, causing memory corruptions. > Did you see it in practice? Or is this patch based on code review? What > will happen if those queued mbufs find their ifq_so NULL? I have a packet trace that triggers this problem when it is injected into the guest's NIC. I am still not quite sure why this happens, but suspect that it could be caused by malformed/partial TCP/IP streams. Setting ifq_so to NULL shouldn't be an issue because there seems to be checks for that (e.g., in if.c). It doesn't seem to affect normal web browsing/downloading in the guest. Vitaly > >> Signed-off-by: Vitaly Chipounov >> --- >> slirp/socket.c | 29 + >> 1 file changed, 29 insertions(+) >> >> diff --git a/slirp/socket.c b/slirp/socket.c >> index 77b0c98..8a7adc8 100644 >> --- a/slirp/socket.c >> +++ b/slirp/socket.c >> @@ -55,6 +55,33 @@ socreate(Slirp *slirp) >>return(so); >> } >> >> +/** >> + * It may happen that a socket is still referenced in various >> + * mbufs at the time it is freed. Clear all references to the >> + * socket here. >> + */ >> +static void soremovefromqueues(struct socket *so) >> +{ >> +if (!so->so_queued) { >> +return; >> +} >> + >> +Slirp *slirp = so->slirp; >> +struct mbuf *ifm; >> + >> +for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = >> ifm->ifq_next) { > This line and the one below smells like checkpatch.pl wasn't run against > it. Granted, slirp patches easy make checkpatch upset as the input is > not properly formatted, but please don't add more violations. > >> +if (ifm->ifq_so == so) { >> +ifm->ifq_so = NULL; >> +} >> +} >> + >> +for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = >> ifm->ifq_next) { >> +if (ifm->ifq_so == so) { >> +ifm->ifq_so = NULL; >> +} >> +} >> +} >> + >> /* >> * remque and free a socket, clobber cache >> */ >> @@ -79,6 +106,8 @@ sofree(struct socket *so) >>if(so->so_next && so->so_prev) >> remque(so); /* crashes if so is not in a queue */ >> >> + soremovefromqueues(so); >> + >>free(so); >> } >> >> > Sorry for the late feedback, > Jan >